Thanks Kenn and Reuven. Based on your feedback, I amended to the PR [1] implementing the missing calls to teardown.
Best, michel [1] https://github.com/apache/beam/pull/8495 On Tue, May 7, 2019 at 6:09 AM Kenneth Knowles <[email protected]> wrote: > > > On Mon, May 6, 2019 at 2:19 PM Reuven Lax <[email protected]> wrote: > >> >> >> On Mon, May 6, 2019 at 2:06 PM Kenneth Knowles <[email protected]> wrote: >> >>> The specification of TearDown is that it is best effort, certainly. >>> >> >> Though I believe the intent of that specification was that a runner will >> call it as long as the process itself has not crashed. >> > > Yea, exactly. Or more abstractly that a runner will call it unless it is > impossible. If the hardware fails, a meteor strikes, etc, then teardown > will not be called. But in normal operation, particularly when the user > code throws a recoverable exception, it should be called. > > Kenn > > >> >> >>> If your runner supports it, then the test is good to make sure there is >>> not a regression. If your runner has partial support, that is within spec. >>> But the idea of the spec is more than you might have such a failure that it >>> is impossible to call the method, not simply never trying to call it. >>> >>> I think it seems to match what we do elsewhere to leave the test, add an >>> annotation, make a note in the capability matrix about the limitation on >>> ParDo. >>> >>> Kenn >>> >>> On Mon, May 6, 2019 at 5:45 AM Michael Luckey <[email protected]> >>> wrote: >>> >>>> Hi, >>>> >>>> after stumbling upon [1] and trying to implement a fix [2], >>>> ParDoLifeCycleTest are failing for >>>> direct runner, spark validatesRunnerBatch and flink >>>> validatesRunnerBatch fail as DoFns teardown is not invoked, if DoFns setup >>>> throw an exceptions. >>>> >>>> This seems to be in line with the specification [3], as this explicitly >>>> states that 'teardown might not be called if unnecessary as processed will >>>> be killed anyway'. >>>> >>>> No I am a bit lost on how to resolve this situation. Currently, we seem >>>> to have following options >>>> - remove the test, although it seems valuable in different (e.g. >>>> streaming?) cases >>>> - to satisfy the test implement the call to teardown in runners >>>> although it seems unnecessary >>>> - add another annotation @CallsTeardownAfterFailingSetup, >>>> @UsesFullParDoLifeCycle or such (would love to get suggestions for >>>> better name here) >>>> - ? >>>> >>>> Thoughts? >>>> >>>> Best, >>>> >>>> michel >>>> >>>> >>>> >>>> [1] https://issues.apache.org/jira/browse/BEAM-7197 >>>> [2] https://github.com/apache/beam/pull/8495 >>>> [3] >>>> https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/DoFn.java#L676-L680 >>>> >>>
