Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cancel() raises Submitexception? #134

Open
andre-merzky opened this issue Apr 17, 2021 · 5 comments
Open

cancel() raises Submitexception? #134

andre-merzky opened this issue Apr 17, 2021 · 5 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@andre-merzky
Copy link
Collaborator

andre-merzky commented Apr 17, 2021

The spec defines:

void cancel(Job job) throws SubmitException

That looks wrong. Also, the text does not clarify when an exception should be raised. I assume that an InvalidStateException should be raised if cancel() is called on a job in final state?

The above also applies to the job.cancel() method.

BTW: we have cancel() on the job executor and the job (which I like). I still think we should have wait() on both also.

@andre-merzky andre-merzky added enhancement New feature or request question Further information is requested labels Apr 17, 2021
@hategan
Copy link
Collaborator

hategan commented Apr 17, 2021

SubmitException on cancel() is meant to convey problems communicating the request to the LRM. Such as qdel failing. So it's not quite a mistake.

InvalidStateException when the job cannot be canceled makes sense.

@andre-merzky
Copy link
Collaborator Author

andre-merzky commented Apr 18, 2021

SubmitException on cancel() is meant to convey problems communicating the request to the LRM. Such as qdel failing. So it's not quite a mistake.

From the spec:

SubmitException
This exception is thrown when the JobExecutor.submit call fails 
for a reason that is independent of the job that is being submitted.

That seems inconsistent with the use in cancel() - it specifically refers to the submit() call, not to 'submitting' any request to the backend. I think it would be confusing to mix the two. If both calls are to share an exception to cover problems with backend communication, then it should maybe be named BackendException or something similar which makes the intent clear.

@hategan
Copy link
Collaborator

hategan commented Apr 18, 2021

I agree it is confusing. There are two options I can think of: 1) rename SubmitException in a way that captures the common denominator between submit and cancel while appropriately modifying the description and 2) Add a cancel specific exception.

I think the exception should be functional rather than nominal, in the same way that IOError is thrown by many callables rather than each callable defining a separate exception based on the operation that is being performed. So that seems to to hint at (1).

@andre-merzky
Copy link
Collaborator Author

I am also for option (1), and BackendException sounds reasonable to me.

@hategan
Copy link
Collaborator

hategan commented Apr 22, 2021

BackendException

Sounds fine I think. Part of me thinks that the exception name should reflect the nature of the problem rather than the location of the problem, since the latter is too similar to SubmitException. That said, I've seen RemoteException quite a few times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants