You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/06/11 20:25:22 UTC
[GitHub] [beam] y1chi opened a new pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
y1chi opened a new pull request #11993:
URL: https://github.com/apache/beam/pull/11993
…s to accomodate Dataflow runner
Dataflow runner backend process Teststream events at second granularity. If advanceWatermark or advanceProcessingTime is set finer than second the Teststream does not advance at all.
Change advanceProcessingTime duration relative to the unix epoch since the processing time timers are set based on realtime.
**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)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Samza | Spark
--- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r442538527
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
My issue is that this changes the test such that the boundary condition is looser by making END_OF_GLOBAL_WINDOW looser.
@acrites Any ideas/suggestions here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r444343155
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
@acrites and I chatted about some of the tests. There is an option to alter the windmill wm_work_set_watermark_resolution_usec and dataflow_watermark_resolution_usec so that the AdvanceWatermark and AdvanceProcessingTime can deal with milliseconds. But it requires additional test framework plumbing. Should we keep this test as it was and try to unsickbay it after we can set the windmill flag values?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r442420147
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
My understanding is that it's trying to test setting a timer with timestamp that exceed END_OF_GLOBAL_WINDOW and the fire timestamp gets altered to the END_OF_GLOBAL_WINDOW(gc time). How the original exceeding-limit timestamp is obtained shouldn't make a difference?
Since END_OF_GLOBAL_WINDOW is with millis granularity, advance the watermark of the TestStream to this time will not be processed by dataflow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #11993:
URL: https://github.com/apache/beam/pull/11993#issuecomment-645764255
R: @aaltay
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #11993:
URL: https://github.com/apache/beam/pull/11993#issuecomment-642911368
R: @acrites @robertwb
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r442563703
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
we can also explicitly set timer with absolute time equal to the same fire time the with original `timer.align(Duration.standardDays(1)).setRelative();`, would that make sense?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r442538527
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
My issue is that this changes the test such that the boundary condition is looser by making END_OF_GLOBAL_WINDOW looser which allows for off by one errors to not be detected.
@acrites Any ideas/suggestions here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r442378176
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
Updating other tests to second granularity makes sense but changing the test to see how END_OF_GLOBAL_WINDOW is interpreted changes what this test is explicitly trying to do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r442563703
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
we can also explicitly set timer with absolute time TIMESTAMP_MAX_VALUE, which set the timer fire time the same to original `timer.align(Duration.standardDays(1)).setRelative();`, would that make sense?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #11993:
URL: https://github.com/apache/beam/pull/11993#issuecomment-644229972
R: @angoenka
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r442420147
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
My understanding is that it's trying to test setting a timer with timestamp that exceed END_OF_GLOBAL_WINDOW and the fire timestamp gets altered to the END_OF_GLOBAL_WINDOW(gc time). How the original exceeding-limit timestamp is obtained(and whether it is set exactly at END_OF_GLOBAL_WINDOW or later than END_OF_GLOBAL_WINDOW) shouldn't make a difference?
Since END_OF_GLOBAL_WINDOW is with millis granularity, advance the watermark of the TestStream to this time will not be processed by dataflow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r445826286
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3817,24 +3817,15 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
- // minus 775 milliseconds to make the grainularity to second as required by dataflow
- // for TestStream.
- .advanceWatermarkTo(
- BoundedWindow.TIMESTAMP_MAX_VALUE
- .minus(Duration.standardDays(1))
- .minus(BoundedWindow.TIMESTAMP_MAX_VALUE.getMillis() % 1000))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
+ .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
Review comment:
nit: use GlobalWindow.INSTANCE.maxTimestamp():
https://github.com/apache/beam/blob/7269e3f8e2b7fb185347f8fcb80f0cd5afcbaeb6/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/GlobalWindow.java#L42
Here and below instead of calculating it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #11993:
URL: https://github.com/apache/beam/pull/11993#issuecomment-644909692
gentle ping for review
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] acrites commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
acrites commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r444339102
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
Yichi and I played around with some flag settings in Dataflow and I think we can change the time resolution for tests to milliseconds, which should make this test pass. However, I'm a little worried in that we then aren't testing the same as prod, so we might end up missing some other bugs that come up.
Otherwise, I don't really know a good way around this since `BoundedWindow.TIMESTAMP_MAX_VALUE` is not a round number of seconds.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r445826286
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3817,24 +3817,15 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
- // minus 775 milliseconds to make the grainularity to second as required by dataflow
- // for TestStream.
- .advanceWatermarkTo(
- BoundedWindow.TIMESTAMP_MAX_VALUE
- .minus(Duration.standardDays(1))
- .minus(BoundedWindow.TIMESTAMP_MAX_VALUE.getMillis() % 1000))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
+ .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
Review comment:
nit: use GlobalWindow.INSTANCE.maxTimestamp():
https://github.com/apache/beam/blob/7269e3f8e2b7fb185347f8fcb80f0cd5afcbaeb6/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/GlobalWindow.java#L42
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r445836802
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3817,24 +3817,15 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
- // minus 775 milliseconds to make the grainularity to second as required by dataflow
- // for TestStream.
- .advanceWatermarkTo(
- BoundedWindow.TIMESTAMP_MAX_VALUE
- .minus(Duration.standardDays(1))
- .minus(BoundedWindow.TIMESTAMP_MAX_VALUE.getMillis() % 1000))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
+ .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lukecwik commented on a change in pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
lukecwik commented on a change in pull request #11993:
URL: https://github.com/apache/beam/pull/11993#discussion_r445817767
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/ParDoTest.java
##########
@@ -3808,15 +3817,24 @@ public void onTimer(
TestStream<KV<String, Integer>> stream =
TestStream.create(KvCoder.of(StringUtf8Coder.of(), VarIntCoder.of()))
// See GlobalWindow,
- // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1))
- .advanceWatermarkTo(BoundedWindow.TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)))
+ // END_OF_GLOBAL_WINDOW is TIMESTAMP_MAX_VALUE.minus(Duration.standardDays(1)),
Review comment:
Lets leave this one tests as is and remove it from the change. We can update the others with the changes since they won't change the semantics of what is being tested.
For Dataflow's TestStream implementation, to simulate production, it should round all the advance timestampts to the closest `>=` internal timestamp. It should also make sure that no two advance timestamps that aren't the same are not rounded to the same value. I believe this would make this test work and it would simulate what is happening in production.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] angoenka merged pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
angoenka merged pull request #11993:
URL: https://github.com/apache/beam/pull/11993
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #11993: Change time granularity to seconds in ParDoTest TestStream timer test…
Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #11993:
URL: https://github.com/apache/beam/pull/11993#issuecomment-649823298
retest this please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org