CADC-12563 expect the PackageRunner job to be SUSPENDED instead of QUEUED #97
Conversation
There was a problem hiding this comment.
for caom2-pkg-server, clients directly invoke the package endpoint so those are going to do the normal PENDING -> QUEUED before being invoked. Unfortunately, that means the PackageRunner needs to tolerate both of these...
You probably need to add something like
protected ExecutionPhase getInitialPhase() {
return ExecutionPhase.QUEUED;
}
and then have the vospace impl override that.
|
Forget about the caom2-pkg-server. Added the method to the PackageRunner, and overrode it in the VospacePackageRunner, and the cavern int-test for packages passes. |
| @@ -206,7 +210,7 @@ private void doIt() { | |||
| } catch (Throwable t) { | |||
| if (ThrowableUtil.isACause(t, InterruptedException.class)) { | |||
| try { | |||
| ep = jobUpdater.setPhase(job.getID(), ExecutionPhase.QUEUED, ExecutionPhase.EXECUTING, new Date()); | |||
| ep = jobUpdater.setPhase(job.getID(), getInitialPhase(), ExecutionPhase.EXECUTING, new Date()); | |||
There was a problem hiding this comment.
this is something that was like this and I never noticed before but it's really broken:
this change from initial to executing in the error handling doesn't make any sense
should be EXECUTING to ERROR with an error message from the throwable and should always call sendError (I don't know what scenario that ABORTED is meant to handle)
out of scope for now: there is also no code here at differentiate the cause and chose an approp status code - that would normally be done by earlier catch blocks and this is the final "unexpected" aka 500 handling... please create an issue for this
There was a problem hiding this comment.
Shouldn't the catch for the throwable just be setting the phase to ERROR and calling sendError(), without checking if the throwable cause is a particular exception?
There was a problem hiding this comment.
yes, as long as the fail is not from doing initial->executing you can be certain that the current phase is executing and go to error.
and because this is really catch(Throwable unexpected) the response code should be 500 in this block.
There was a problem hiding this comment.
in the throwable you can get the current job phase when setting the phase to error, which would cover the phase in initial or executing?
|
|
||
| } | ||
| try { | ||
| ep = jobUpdater.setPhase(job.getID(), job.getExecutionPhase(), ExecutionPhase.ERROR, new Date()); |
There was a problem hiding this comment.
job.getExecutionPhase( ) is not reliably up-to-date (it's probably still SUSPENDED in this case)... I think you can hard-code EXECUTING here because logically nothing happens before SUSPENDED->EXECUTING or after EXECUTING->COMPETED.
there really should be a catch designed to handle those setPhase operations throwing a job-related exception, but because there is a catch(Throwable) your IDE doesn't tell you about what those are... out of scope. Just hard code EXECUTING->ERROR
Also, for the sendError, it was using the two-arg method so that "unexpected failure" would be added to the output. Please revert to that.
No description provided.