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 >>> >>
