Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

convert windmill min timestamp to beam min timestamp #21740

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

Naireen
Copy link
Contributor

@Naireen Naireen commented Jun 8, 2022

Please add a meaningful description for your change here


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Add a link to the appropriate issue in your description, if applicable. This will automatically link the pull request to the issue.

@asf-ci
Copy link

asf-ci commented Jun 8, 2022

Can one of the admins verify this patch?

1 similar comment
@asf-ci
Copy link

asf-ci commented Jun 8, 2022

Can one of the admins verify this patch?

@Naireen
Copy link
Contributor Author

Naireen commented Jun 8, 2022

R: @scwhittle

@Naireen
Copy link
Contributor Author

Naireen commented Jun 10, 2022

Run Java PreCommit

1 similar comment
@scwhittle
Copy link
Contributor

Run Java PreCommit

Copy link
Contributor

@scwhittle scwhittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failure appeared unrelated BQ rate limiting, triggering rerun

@kennknowles as a possible merger

@aaltay
Copy link
Member

aaltay commented Jun 14, 2022

Run Java PreCommit

2 similar comments
@aaltay
Copy link
Member

aaltay commented Jun 14, 2022

Run Java PreCommit

@kennknowles
Copy link
Member

Run Java PreCommit

@kennknowles kennknowles self-assigned this Jun 14, 2022
@aaltay
Copy link
Member

aaltay commented Jun 15, 2022

Run Java PreCommit

@aaltay
Copy link
Member

aaltay commented Jun 15, 2022

@kennknowles - precommit flaked 5 times in a row. We need to do something about it.

@kennknowles
Copy link
Member

Yes. We have someone already working on "org.apache.beam.sdk.io.pulsar.PulsarIOTest.testReadFromSimpleTopic: Trying to claim offset 1655305408194 while last attempted was 1655305409570" but I think it just needs to be disabled. I also saw a lot of docker issues. Tests that use a local server in a docker container should probably be moved off of the main job. Because the job uses gradle's buildDependents it is hard to make fine-grained adjustments. I'm thinking about it.

@aaltay
Copy link
Member

aaltay commented Jun 15, 2022

Yes. We have someone already working on "org.apache.beam.sdk.io.pulsar.PulsarIOTest.testReadFromSimpleTopic: Trying to claim offset 1655305408194 while last attempted was 1655305409570" but I think it just needs to be disabled. I also saw a lot of docker issues. Tests that use a local server in a docker container should probably be moved off of the main job. Because the job uses gradle's buildDependents it is hard to make fine-grained adjustments. I'm thinking about it.

Thank you.

I think I saw other issues as well. But I did not save the jenkins links. IO was the general root cause.

@aaltay aaltay merged commit 2104960 into apache:master Jun 15, 2022
@kennknowles
Copy link
Member

Hmm this is green but in https://ci-beam.apache.org/view/PostCommit/job/beam_PreCommit_Java_Cron/5560/ and all subsequent tests (including current PRs) we are seeing org.apache.beam.runners.dataflow.worker.WindmillTimerInternalsTest.testTimerDataToFromTimer fail with

Expected: <TimerData{timerId=0:-9223372036854775, timerFamilyId=, namespace=Global, timestamp=-290308-12-21T19:59:05.225Z, outputTimestamp=-290308-12-21T19:59:05.224Z, domain=EVENT_TIME, deleted=false}>
     but: was <TimerData{timerId=0:-9223372036854775, timerFamilyId=, namespace=Global, timestamp=-290308-12-21T19:59:05.225Z, outputTimestamp=-290308-12-21T19:59:05.225Z, domain=EVENT_TIME, deleted=false}>

and based on timing and code touched this PR seems the most likely to be related. TBH I don't get it since this seems like it is deterministic and would have been red on this PR

@Abacn
Copy link
Contributor

Abacn commented Jun 16, 2022

"timestamp=-290308-12-21T19:59:05.225Z" is a negative year. It is an overflow. Looks like the conversion treated overflow integers in different way.

@@ -45,6 +45,9 @@ public static Instant windmillToHarnessTimestamp(long timestampUs) {
// Windmill should never send us an unknown timestamp.
Preconditions.checkArgument(timestampUs != Long.MIN_VALUE);
Instant result = new Instant(divideAndRoundDown(timestampUs, 1000));
if (result.isBefore(BoundedWindow.TIMESTAMP_MIN_VALUE)) {
return BoundedWindow.TIMESTAMP_MIN_VALUE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition prevented timestamp overflow and broke unit test WindmillTimerInternalsTest.testTimerDataToFromTimer(=BoundedWindow.TIMESTAMP_MIN_VALUE). If this is intended behavior, should adjust the test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants