TheNeuralBit commented on a change in pull request #16035:
URL: https://github.com/apache/beam/pull/16035#discussion_r759694192
##########
File path:
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/PipelineTranslationTest.java
##########
@@ -105,18 +102,12 @@ public void process(ProcessContext c) {
BigEndianLongCoder customCoder = BigEndianLongCoder.of();
PCollection<Long> elems =
complexPipeline.apply(GenerateSequence.from(0L).to(207L));
PCollection<Long> counted =
elems.apply(Count.globally()).setCoder(customCoder);
- PCollection<Long> windowed =
- counted.apply(
- Window.<Long>into(FixedWindows.of(Duration.standardMinutes(7)))
- .triggering(
- AfterWatermark.pastEndOfWindow()
- .withLateFirings(AfterPane.elementCountAtLeast(19)))
- .accumulatingFiredPanes()
- .withAllowedLateness(Duration.standardMinutes(3L)));
- final WindowingStrategy<?, ?> windowedStrategy =
windowed.getWindowingStrategy();
- PCollection<KV<String, Long>> keyed = windowed.apply(WithKeys.of("foo"));
- PCollection<KV<String, Iterable<Long>>> grouped =
keyed.apply(GroupByKey.create());
Review comment:
It looks like this test is trying to create a generic pipeline that
exercises a variety of features, so we can test translating to/from proto. It's
probably beneficial to still apply the `WithKeys` and `GroupByKey` transforms
(but we don't need to track the resulting PCollections).
##########
File path:
runners/direct-java/src/test/java/org/apache/beam/runners/direct/SideInputContainerTest.java
##########
@@ -480,6 +480,10 @@ private CountDownLatch invokeLatchedCallback(
invocation -> {
Object callback = invocation.getArguments()[3];
final Runnable callbackRunnable = (Runnable) callback;
+ // Removing unused variable throws new errorprone warnings:
FutureReturnValueIgnored
+ // and
+ // CheckReturnValue
+ @SuppressWarnings("unused")
Review comment:
Let's suppress the other warnings rather than suppressing unused - those
warnings better indicate the problem here.
##########
File path:
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/graph/QueryablePipelineTest.java
##########
@@ -428,18 +428,5 @@ public void retainOnlyPrimitivesIgnoresUnreachableNodes() {
.apply(MapElements.into(TypeDescriptors.longs()).via(l -> l +
1));
}
});
-
- Components augmentedComponents =
- PipelineTranslation.toProto(p)
- .getComponents()
- .toBuilder()
- .putCoders("extra-coder", RunnerApi.Coder.getDefaultInstance())
- .putWindowingStrategies(
- "extra-windowing-strategy",
RunnerApi.WindowingStrategy.getDefaultInstance())
- .putEnvironments("extra-env",
RunnerApi.Environment.getDefaultInstance())
- .putPcollections("extra-pc",
RunnerApi.PCollection.getDefaultInstance())
- .build();
- Collection<String> primitiveComponents =
- QueryablePipeline.getPrimitiveTransformIds(augmentedComponents);
Review comment:
I think we should just remove this whole test. It looks like the
assertions were removed in https://github.com/apache/beam/pull/4916, so it's
not really testing anything, and the comment on L417 indicates the behavior it
was originally testing isn't critical anyway.
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/AssignWindowsParDoFnFactory.java
##########
@@ -68,14 +68,12 @@ public ParDoFn create(
}
private static class AssignWindowsParDoFn<T, W extends BoundedWindow>
implements ParDoFn {
- private final DataflowExecutionContext.DataflowStepContext stepContext;
private final WindowFn<T, W> windowFn;
private @Nullable Receiver receiver;
AssignWindowsParDoFn(
WindowFn<T, W> windowFn, DataflowExecutionContext.DataflowStepContext
stepContext) {
Review comment:
Isn't the `stepContext` argument also unused now? It's curious that that
doesn't raise a warning too.
##########
File path:
runners/core-java/src/test/java/org/apache/beam/runners/core/SimplePushbackSideInputDoFnRunnerTest.java
##########
@@ -319,15 +319,12 @@ public void testOnTimerCalled() {
}
private static class TestDoFnRunner<InputT, OutputT> implements
DoFnRunner<InputT, OutputT> {
- private final Coder<InputT> inputCoder;
List<WindowedValue<InputT>> inputElems;
List<TimerData> firedTimers;
private boolean started = false;
private boolean finished = false;
- TestDoFnRunner(Coder<InputT> inputCoder) {
- this.inputCoder = inputCoder;
- }
+ TestDoFnRunner() {}
Review comment:
nit: we can just not define this constructor, it will be created by
default
```suggestion
```
--
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]
