[BEAM-3646] Add Javadoc on how Teardown is best-effort #4637
Conversation
Assumptions that Teardown will be called both in the case of an orderly shutdown or in a non-orderly shutdown are not safe, and this assumption can lead to pipelines which do not produce desired side effects in all cases.
|
Reviewed 2 of 2 files at r1. Comments from Reviewable |
|
run java gradle precommit |
| * Annotation for the method to use to clean up this instance before it is discarded. No other | ||
| * method will be called after a call to the annotated method is made. | ||
| * | ||
| * <p>Note that calls to the annotated method are best effort, and may not occur for arbitrary |
There was a problem hiding this comment.
What would it cost to not make it best effort?
Basically an API which is "best effort" is quite pointless and since beam must not assume the JVM is not reused (it is by some runners) then beam should ensure the transforms can release whatever they used so enforce the call to teardown whatever happent, no?
There was a problem hiding this comment.
It is impossible to make it guaranteed, as a machine may fail at any time for any reason. Users should never depend on invocation of teardown to produce results.
Teardown being reliable when there are no failures is possible, but that's a combination of per-runner and per-harness behavior, which I've filed https://issues.apache.org/jira/browse/BEAM-3704 to enable
There was a problem hiding this comment.
Ok so the javadoc should mention it can be missed due to a crash but except that it is guaranteed. Any code can be dropped due to a crash like the finish bundle you recommand in this javadoc so it is quite misleading now and makes the api not that helpful from a user perspective :(.
There was a problem hiding this comment.
I assume the purpose here is to guide the end user to use tearDown method appropriately. I think it is better to assume the user is beginner and does not know much about how the the job is executed. May be better to emphasis 'best effort' more and (say something to the effect of 'usually invoked during a orderly shutdown'), possibly remove the phrase 'may not occur for arbitrary reasons'. Just thinking aloud.. non-binding review comment.
Assumptions that Teardown will be called both in the case of an orderly
shutdown or in a non-orderly shutdown are not safe, and this assumption
can lead to pipelines which do not produce desired side effects in all
cases.
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.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.