search

ExaWorks / job-api-spec

https://exaworks.org/job-api-spec/
3 stars 3 forks source link

`cancel()` raises `Submitexception`? #134

Open andre-merzky opened 3 years ago

andre-merzky commented 3 years ago

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.

hategan commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

hategan commented 3 years ago

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.