[BEAM-9557] Fix timer window boundary checking

[reuvenlax]

R: @steveniemitz

[steveniemitz]

can we add a unit test to validate this behavior as well?

[amaliujia]

Run Java PreCommit

[amaliujia]

This failed test might be relevant to this PR: org.apache.beam.sdk.transforms.ParDoTest$TimerTests.testOutOfBoundsEventTimeTimer

(link https://builds.apache.org/job/beam_PreCommit_Java_Phrase/1930/)

[reuvenlax]

@amaliujia appears that test looks for specific strings in the exception (always a recipe for a brittle test). I changed "event time timer" to "event-time timer" which broke that test. Will fix.

[aaltay]

@amaliujia appears that test looks for specific strings in the exception (always a recipe for a brittle test). I changed "event time timer" to "event-time timer" which broke that test. Will fix.

Agreed, it is brittle test.

@reuvenlax are you planning to fix the test, or could @amaliujia send a fixup commit here to change "event-time" to "event time"? This is only release blocking issue currently, I would like to unblock it sooner.

[reuvenlax]

@aaltay fixing this now

[reuvenlax]

@aaltay added new unit tests and fixed the broken one. However I'm noticing that test triggering seems broken on a number of PRs now. Is something wrong with the Jenkins master?

[aaltay]

@aaltay added new unit tests and fixed the broken one. However I'm noticing that test triggering seems broken on a number of PRs now. Is something wrong with the Jenkins master?

Yes. Ongoing issue. @yifanzou is working on it. Repeating the test triggers a few times usually helps.

[reuvenlax]

retest this please

[reuvenlax]

retest this please

[reuvenlax]

retest this please

[reuvenlax]

@steveniemitz tests added

[steveniemitz]

looks great! These tests will be really helpful to ensure we don't have more regressions in the future. Thanks!

[reuvenlax]

Run Java PreCommit

[reuvenlax]

@aaltay Has part of this change already been merged to master? I notice that SimpleDoFnRunner.java at head already contains these changes, but the unit tests are not it.

[steveniemitz]

it looks like #11252 was merged instead of this one.

[reuvenlax]

I see. We should still merge this so we get the unit-test coverage.

…

On Sun, Mar 29, 2020 at 12:19 PM Steven Niemitz ***@***.***> wrote: it looks like #11252 <#11252> was merged instead of this one. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11226 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAYJVMT6FWXML2TDA2X4JLRJ6NKRANCNFSM4LTWF4CQ> .

[amaliujia]

LGTM

[amaliujia]

@reuvenlax

do you need help on the failed java tests?