You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@beam.apache.org by Sam Rohde <sr...@google.com> on 2019/01/22 18:20:46 UTC

Magic number explanation in ParDoTest.java

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

Re: Magic number explanation in ParDoTest.java

Posted by Kenneth Knowles <ke...@apache.org>.

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 <sr...@google.com> 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 <kl...@google.com> 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 <sr...@google.com> 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
>>>
>>

Re: Magic number explanation in ParDoTest.java

Posted by Sam Rohde <sr...@google.com>.

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 <kl...@google.com> 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 <sr...@google.com> 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
>>
>

Re: Magic number explanation in ParDoTest.java

Posted by Kenneth Knowles <kl...@google.com>.

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 <sr...@google.com> 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
>