scwhittle commented on a change in pull request #16840:
URL: https://github.com/apache/beam/pull/16840#discussion_r805587904
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/GroupByKeyTest.java
##########
@@ -47,25 +47,14 @@
import org.apache.beam.sdk.coders.MapCoder;
import org.apache.beam.sdk.coders.StringUtf8Coder;
import org.apache.beam.sdk.coders.VarIntCoder;
-import org.apache.beam.sdk.testing.LargeKeys;
-import org.apache.beam.sdk.testing.NeedsRunner;
-import org.apache.beam.sdk.testing.PAssert;
-import org.apache.beam.sdk.testing.TestPipeline;
-import org.apache.beam.sdk.testing.TestStream;
-import org.apache.beam.sdk.testing.UsesTestStreamWithProcessingTime;
-import org.apache.beam.sdk.testing.ValidatesRunner;
+import org.apache.beam.sdk.io.GenerateSequence;
+import org.apache.beam.sdk.state.TimeDomain;
+import org.apache.beam.sdk.state.Timer;
+import org.apache.beam.sdk.state.TimerSpec;
+import org.apache.beam.sdk.state.TimerSpecs;
+import org.apache.beam.sdk.testing.*;
import org.apache.beam.sdk.transforms.display.DisplayData;
-import org.apache.beam.sdk.transforms.windowing.AfterPane;
-import org.apache.beam.sdk.transforms.windowing.AfterProcessingTime;
-import org.apache.beam.sdk.transforms.windowing.AfterWatermark;
-import org.apache.beam.sdk.transforms.windowing.FixedWindows;
-import org.apache.beam.sdk.transforms.windowing.GlobalWindow;
-import org.apache.beam.sdk.transforms.windowing.IntervalWindow;
-import org.apache.beam.sdk.transforms.windowing.Repeatedly;
-import org.apache.beam.sdk.transforms.windowing.Sessions;
-import org.apache.beam.sdk.transforms.windowing.SlidingWindows;
-import org.apache.beam.sdk.transforms.windowing.TimestampCombiner;
-import org.apache.beam.sdk.transforms.windowing.Window;
+import org.apache.beam.sdk.transforms.windowing.*;
Review comment:
Just checking about *. I haven't seen that elsewhere in Beam. Is it
desirable, did allowing that change?
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/GroupByKeyTest.java
##########
@@ -186,6 +176,112 @@ public void testCombiningAccumulatingProcessingTime()
throws Exception {
p.run();
}
+ /**
+ * Tests that data from a processing time trigger flows through subsequent
GroupByKey
+ * transforms. To test this with TestStream, we check that it arrives in
an early pane,
Review comment:
I don't see where you check it is in the early pane in the test. If it's
there maybe a comment would help.
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/GroupByKeyTest.java
##########
@@ -186,6 +176,112 @@ public void testCombiningAccumulatingProcessingTime()
throws Exception {
p.run();
}
+ /**
+ * Tests that data from a processing time trigger flows through subsequent
GroupByKey
+ * transforms. To test this with TestStream, we check that it arrives in
an early pane,
+ * demonstrating that the watermark did not cause the output. No runner
appears to support
+ * {@link TestStream} processing time correctly for this, so we do it with
actually processing
+ * time delays, which must be kept small so the test suite does not take
forever. There are also
+ * bugs in runners around {@code Window.configure()} behaving differently
than {@code
Review comment:
should there be a JIRA for those bugs and referenced 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
