TheNeuralBit commented on a change in pull request #15742:
URL: https://github.com/apache/beam/pull/15742#discussion_r745195929
##########
File path:
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/CombineTranslationTest.java
##########
@@ -188,9 +187,8 @@ public void leaveCompositeTransform(Node node) {
SdkComponents sdkComponents = SdkComponents.create();
sdkComponents.registerEnvironment(Environments.createDockerEnvironment("java"));
- CombinePayload payload =
-
CombineTranslation.CombineGloballyPayloadTranslator.payloadForCombineGlobally(
- (AppliedPTransform) combine.get(), sdkComponents);
+
CombineTranslation.CombineGloballyPayloadTranslator.payloadForCombineGlobally(
+ (AppliedPTransform) combine.get(), sdkComponents);
Review comment:
Can we just remove this? Or does it have some side effect?
##########
File path:
runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ReadTranslation.java
##########
@@ -79,6 +79,7 @@ public static FunctionSpec toProto(Source<?> source,
SdkComponents components) {
}
}
+ @SuppressWarnings("unused")
Review comment:
This warning (and the other one in this file) seems to be referring to
the `components` argument. Please just remove that argument, it looks like we
removed the need for it in
https://github.com/apache/beam/commit/edff83f1a7326993f809f1e372b8b54d030b90cc,
but we didn't drop the unused argument then.
##########
File path:
runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowPipelineTranslator.java
##########
@@ -276,6 +276,7 @@ public static String jobToString(Job job) {
* purposely wrapped inside of a {@link Supplier} to prevent the incorrect
usage of the {@link
* AtomicLong} that is contained.
*/
+ @SuppressWarnings("unused")
Review comment:
Can we just remove `idGenerator`?
##########
File path:
runners/google-cloud-dataflow-java/worker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingGroupAlsoByWindowsReshuffleDoFnTest.java
##########
@@ -198,6 +198,7 @@ public void testFixedWindows() throws Exception {
return makeRunner(outputTag, outputManager, windowingStrategy, fn);
}
+ @SuppressWarnings("unused")
Review comment:
I think this can just be removed
##########
File path:
runners/core-java/src/test/java/org/apache/beam/runners/core/SimpleDoFnRunnerTest.java
##########
@@ -391,6 +391,7 @@ public void testInfiniteSkew() {
static final String TIMER_ID = "throwingTimerId";
+ @SuppressWarnings("unused")
Review comment:
It would be nice if we could somehow make `@TimerId` and `@StateId`
automatically apply `@SuppressWarnings("unused")` somehow... No idea how that
might work though. Maybe @kennknowles has an idea.
##########
File path:
runners/core-java/src/main/java/org/apache/beam/runners/core/StateTags.java
##########
@@ -47,6 +47,7 @@
})
public class StateTags {
+ @SuppressWarnings("unused")
Review comment:
Can this just be removed?
##########
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(Coder<InputT> inputCoder) {}
Review comment:
It looks like there's only one usage of this constructor. Please go
ahead and update that usage to just call the 0-argument constructor, and remove
this one.
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/graph/DeduceFlattenLocationsFunction.java
##########
@@ -156,8 +156,8 @@
// Find all predecessor and successor locations for every flatten.
for (Node flatten : flattens) {
- AggregatedLocation predecessorLocations = AggregatedLocation.NEITHER;
- AggregatedLocation successorLocations = AggregatedLocation.NEITHER;
+ AggregatedLocation predecessorLocations;
+ AggregatedLocation successorLocations;
Review comment:
Please combine these two lines with the next two lines
##########
File path:
runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowPipelineTranslator.java
##########
@@ -1180,11 +1177,9 @@ public void translate(
translateInputs(
stepContext, context.getInput(transform),
transform.getSideInputs(), context);
translateOutputs(context.getOutputs(transform), stepContext);
- String ptransformId =
-
context.getSdkComponents().getPTransformIdOrThrow(context.getCurrentTransform());
+
context.getSdkComponents().getPTransformIdOrThrow(context.getCurrentTransform());
Review comment:
Can't we just remove all of these?
##########
File path:
runners/google-cloud-dataflow-java/worker/src/test/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorkerTest.java
##########
@@ -799,6 +799,7 @@ public void testHotKeyLogging() throws Exception {
makeInput(i, TimeUnit.MILLISECONDS.toMicros(i), "key",
DEFAULT_SHARDING_KEY));
}
+ @SuppressWarnings("unused")
Map<Long, Windmill.WorkItemCommitRequest> result =
server.waitForAndGetCommits(numIters);
Review comment:
Can we just do this instead? (same for the other suppression in this
file)
```suggestion
server.waitForAndGetCommits(numIters);
```
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/graph/RegisterNodeFunction.java
##########
@@ -146,6 +146,7 @@ public static RegisterNodeFunction withoutPipeline(
null, idGenerator, stateApiServiceDescriptor,
timerApiServiceDescriptor);
}
+ @SuppressWarnings("unused")
Review comment:
Can this be removed?
##########
File path:
runners/google-cloud-dataflow-java/worker/src/test/java/org/apache/beam/runners/dataflow/worker/WindmillStateCacheTest.java
##########
@@ -376,6 +376,7 @@ public void testMultipleFamilies() throws Exception {
cache.forComputation("comp1").forKey(computationKey("comp1", "key1",
SHARDING_KEY), 0L, 0L);
WindmillStateCache.ForKeyAndFamily family1 = keyCache.forFamily("family1");
WindmillStateCache.ForKeyAndFamily family2 = keyCache.forFamily("family2");
+ @SuppressWarnings("unused")
Review comment:
Can this be removed?
##########
File path:
examples/java/src/main/java/org/apache/beam/examples/cookbook/JoinExamples.java
##########
@@ -93,7 +93,7 @@
public void processElement(ProcessContext c) {
KV<String, CoGbkResult> e = c.element();
String countryCode = e.getKey();
- String countryName = "none";
+ String countryName;
Review comment:
Please combine this line with the next one
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/util/common/worker/MapTaskExecutor.java
##########
@@ -72,9 +69,7 @@ public void execute() throws Exception {
// Save the current thread that is executing so that abort() can interrupt
it, we save it before
// starting the progress reporter thread, therefore ensuring thread safety
through implicit
// serialization of events.
- synchronized (this) {
- this.currentExecutorThread = Thread.currentThread();
- }
+ // Current thread that execute() is running on; null when not running.
Review comment:
This whole comment can be removed, it seems to be referring to logic
that no longer exists (`abort` doesn't reference `currentExecutorThread`.
--
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]
