[BEAM-4176] Store and serve termination state after portable job termination #6537
Conversation
| } | ||
|
|
||
| @Override | ||
| public State getState() { | ||
| if(terminationState != null){ |
| @@ -89,6 +95,9 @@ public State waitUntilFinish(Duration duration) { | |||
|
|
|||
| @Override | |||
| public State waitUntilFinish() { | |||
| if(terminationState != null){ | |||
| jobService.close(); | ||
| } catch (Exception e) { | ||
| LOG.warn("Error cleaning up job service", e); | ||
| } |
There was a problem hiding this comment.
Why should we keep the connection to the JobServer if we have reached the final state? Final implies that no other state can be obtained.
There was a problem hiding this comment.
The current behavior of connection cleanup is un-intuitive. The connection use to get closed only if the user called waitUntillFinish while if user did not call WUF then the connection is not closed. Hence removing the connection closing here so that the connection is only closed explicitly in close call.
There was a problem hiding this comment.
I think it would be ok to have a AutoCloseable close method and call it from here to close eagerly when the terminal state has already been reached.
| @Override | ||
| public void close() throws Exception { | ||
| // Close the job service. | ||
| try (CloseableResource<JobServiceBlockingStub> jobService = this.jobService) {} |
There was a problem hiding this comment.
Wonder if we need some cleanup registry in the future to close all these connections when we shutdown the JVM.
There was a problem hiding this comment.
yes, we will need it. Also as the jobServer is not persistent, the PipelineResult object usability is limited.
One approach can be to just keep on requesting for state after job creation.
Once we reach the termination state then close the connection.
There was a problem hiding this comment.
Shall we go with this approach?
There was a problem hiding this comment.
A background thread which does the polling might not be desired by the user. On the other hand, when waitUntilFinished() is called polling is explicitly requested. We would like to do the polling because it helps to find out whether to close the JobServer connection but I think that needs to be handled in a different way, e.g. with a registry of some sort.
There was a problem hiding this comment.
I agree, I would differ this change to a later time.
angoenka
force-pushed
the
java_pvr_waituntilfinish_connection
branch
3 times, most recently
from
cbfd4a2
to
f15adac
Compare
angoenka
force-pushed
the
java_pvr_waituntilfinish_connection
branch
from
f15adac
to
863b904
Compare
The jobserver connection is closed after job is terminated which means user can't check the job status again even when its terminated. Store and provide terminated job status to user.
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.It will help us expedite review of your Pull Request if you tag someone (e.g.
@username) to look at it.Post-Commit Tests Status (on master branch)