OK I dug in to the code. 1. This test should be using TestStream to control the watermark if it is to be a reliable test, since it sets a relative timer. Sorry I missed that in review. It is surprising that this test works on any runner, much less multiple. I wonder if it is blacklisted for most. 2. The application of offset and alignment should be explicitly sequenced since they would yield different results. 3. The result is what it is because Java's % does not implement modular arithmetic: https://stackoverflow.com/questions/4412179/best-way-to-make-javas-modulus-behave-like-it-should-with-negative-numbers but the result is a bug independent of that:
Min timestamp is -9223372036854775 so ignoring most of the digits and inlining the code in SimpleDoFnRunner, you might think: millisSinceStart = (now + offset) % period = (-4775 + 1) % 1000 = 226 So that would be accurately named. But since Java copies C/C++ instead of mathematics, the result is -774. target = now + period - millisSinceStart = -4775 + 1000 - (-774) = -3001 So that's 1774 later, but that doesn't make the answer useful. What I would have expected given the implicit sequencing would be -4000, the result of "set a timer for 1 milli from now, but align it to 1000 milli intervals relative to the epoch". Or in the sequence expressed in the test (not reflected in the data structures) it could mean "align the current (watermark) time to the next 1000 milli since epoch boundary then set a timer for 1 milli later", which would be -3999. FWIW the result of this code using proper modular arithmetic would be -4001 and that number is not a good result either. Kenn On Tue, Jan 22, 2019 at 6:11 PM Sam Rohde <[email protected]> wrote: > Thanks for digging up the PR. I'm still confused as to why that magic > number is still there though. Why is there an expectation that the > timestamp from the timer is *exactly *1774ms past > BoundedWindow.TIMESTAMP_MIN_VALUE? > > On Tue, Jan 22, 2019 at 10:43 AM Kenneth Knowles <[email protected]> wrote: > >> The commit comes from this PR: https://github.com/apache/beam/pull/2273 >> >> Kenn >> >> On Tue, Jan 22, 2019 at 10:21 AM Sam Rohde <[email protected]> wrote: >> >>> Hi all, >>> >>> Does anyone have context why there is a magic number of "1774" >>> milliseconds in the ParDoTest.java on line 2618? This is in >>> the testEventTimeTimerAlignBounded method. >>> >>> File at master: >>> https://github.com/apache/beam/blob/master/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java#L2618 >>> >>> First added commit: >>> https://github.com/apache/beam/commit/4f934923d28798dfe7cd18c86ff4bcf8eebc27e5 >>> >>> Regards, >>> Sam >>> >>
