TheNeuralBit commented on a change in pull request #15742:
URL: https://github.com/apache/beam/pull/15742#discussion_r751527807
##########
File path:
runners/core-java/src/test/java/org/apache/beam/runners/core/triggers/TriggerStateMachinesTest.java
##########
@@ -86,9 +86,7 @@ public void testStateMachineForAfterWatermark() {
RunnerApi.Trigger.newBuilder()
.setAfterEndOfWindow(RunnerApi.Trigger.AfterEndOfWindow.getDefaultInstance())
.build();
- AfterWatermarkStateMachine.FromEndOfWindow machine =
- (AfterWatermarkStateMachine.FromEndOfWindow)
- TriggerStateMachines.stateMachineForTrigger(trigger);
+ TriggerStateMachines.stateMachineForTrigger(trigger);
Review comment:
```suggestion
```
##########
File path:
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/graph/QueryablePipelineTest.java
##########
@@ -439,7 +439,6 @@ public void retainOnlyPrimitivesIgnoresUnreachableNodes() {
.putEnvironments("extra-env",
RunnerApi.Environment.getDefaultInstance())
.putPcollections("extra-pc",
RunnerApi.PCollection.getDefaultInstance())
.build();
- Collection<String> primitiveComponents =
- QueryablePipeline.getPrimitiveTransformIds(augmentedComponents);
+ QueryablePipeline.getPrimitiveTransformIds(augmentedComponents);
Review comment:
```suggestion
```
should be fine to remove this entirely
##########
File path:
runners/direct-java/src/test/java/org/apache/beam/runners/direct/SideInputContainerTest.java
##########
@@ -473,28 +472,28 @@ private void immediatelyInvokeCallback(PCollectionView<?>
view, BoundedWindow wi
* windowing strategy is invoked, start a thread that will invoke the
callback after the returned
* {@link CountDownLatch} is counted down once.
*/
+ @SuppressWarnings({"FutureReturnValueIgnored", "CheckReturnValue"})
Review comment:
is this suppression relevant for this PR?
##########
File path:
runners/google-cloud-dataflow-java/worker/src/test/java/org/apache/beam/runners/dataflow/worker/IsmReaderTest.java
##########
@@ -139,7 +138,7 @@ public void setUp() {
operationContext =
executionContext.createOperationContext(NameContextsForTests.nameContextForTest());
stateCloseable =
executionContext.getExecutionStateTracker().enterState(state);
- sideInputReadCounter = new DataflowSideInputReadCounter(executionContext,
operationContext, 1);
+ new DataflowSideInputReadCounter(executionContext, operationContext, 1);
Review comment:
```suggestion
```
##########
File path:
runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ReadTranslationTest.java
##########
@@ -73,7 +73,7 @@ public void testToFromProtoBounded() throws Exception {
new SplittableParDo.PrimitiveBoundedRead<>(Read.from(boundedSource));
SdkComponents components = SdkComponents.create();
components.registerEnvironment(Environments.createDockerEnvironment("java"));
Review comment:
nit: we should be able to remove this too,
`components.registerEnvironment` doesn't have a relevant side effect
```suggestion
```
##########
File path:
runners/core-java/src/test/java/org/apache/beam/runners/core/triggers/TriggerStateMachineTester.java
##########
@@ -137,10 +133,7 @@ public void injectElements(int... values) throws Exception
{
// Merging requires accumulation mode or early firings can break up a
session.
// Not currently an issue with the tester (because we never GC) but we
don't want
// mystery failures due to violating this need.
- AccumulationMode mode =
- windowFn.isNonMerging()
- ? AccumulationMode.DISCARDING_FIRED_PANES
- : AccumulationMode.ACCUMULATING_FIRED_PANES;
+ windowFn.isNonMerging();
Review comment:
Please just remove this (and above comment) entirely
##########
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,6 @@ 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) {
Review comment:
Please remove the above comment as well
##########
File path:
examples/java/src/main/java/org/apache/beam/examples/complete/game/StatefulTeamScore.java
##########
@@ -77,7 +77,8 @@
* the same topic to which the Injector is publishing.
*/
@SuppressWarnings({
- "nullness" // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
+ "nullness", // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
+ "unused" // TODO: Remove when new version of errorprone is released (2.11.0)
Review comment:
I filed BEAM-13271 specifically for removing these (and assigned it to
myself, I'm happy to take ownership of this minor cleanup), can you reference
it in all of these TODOs?
```suggestion
"unused" // TODO(BEAM-13271): Remove when new version of errorprone is
released (2.11.0)
```
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/graph/RegisterNodeFunction.java
##########
@@ -127,10 +127,8 @@
public static RegisterNodeFunction forPipeline(
RunnerApi.Pipeline pipeline,
IdGenerator idGenerator,
- Endpoints.ApiServiceDescriptor stateApiServiceDescriptor,
- Endpoints.ApiServiceDescriptor timerApiServiceDescriptor) {
- return new RegisterNodeFunction(
- pipeline, idGenerator, stateApiServiceDescriptor,
timerApiServiceDescriptor);
+ Endpoints.ApiServiceDescriptor stateApiServiceDescriptor) {
Review comment:
Hm this method is public, I'm not sure we should change it's signature.
Were you getting an UnusedVariable warning from error-prone here? I thought
error-prone wouldn't raise UnusedVariable for public APIs.
If it is raising a warning here (and other Public APIs), please just
suppress it for now. We need to be careful when changing public APIs to avoid
breaking users.
##########
File path:
sdks/java/testing/nexmark/src/main/java/org/apache/beam/sdk/nexmark/queries/sql/SqlQuery2.java
##########
@@ -51,18 +51,18 @@
private final long skipFactor;
private final Class<? extends QueryPlanner> plannerClass;
- private SqlQuery2(String name, long skipFactor, Class<? extends
QueryPlanner> plannerClass) {
+ private SqlQuery2(long skipFactor, Class<? extends QueryPlanner>
plannerClass) {
super("SqlQuery2");
Review comment:
```suggestion
super(name);
```
I think this name should be plumbed through here instead
##########
File path:
runners/core-java/src/test/java/org/apache/beam/runners/core/triggers/TriggerStateMachineTester.java
##########
@@ -119,10 +118,7 @@ public void injectElements(int... values) throws Exception
{
// Merging requires accumulation mode or early firings can break up a
session.
// Not currently an issue with the tester (because we never GC) but we
don't want
// mystery failures due to violating this need.
- AccumulationMode mode =
- windowFn.isNonMerging()
- ? AccumulationMode.DISCARDING_FIRED_PANES
- : AccumulationMode.ACCUMULATING_FIRED_PANES;
+ windowFn.isNonMerging();
Review comment:
Please just remove this (and above comment) entirely
##########
File path:
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/cep/CEPPattern.java
##########
@@ -65,8 +61,7 @@ public Quantifier getQuantifier() {
return quant;
}
- public static CEPPattern of(
- Schema theSchema, String patternVar, RexCall patternDef, Quantifier
quant) {
- return new CEPPattern(theSchema, patternVar, patternDef, quant);
+ public static CEPPattern of(String patternVar, RexCall patternDef,
Quantifier quant) {
+ return new CEPPattern(patternVar, patternDef, quant);
Review comment:
public API
##########
File path:
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/DynamicProtoCoder.java
##########
@@ -125,11 +125,7 @@ public int hashCode() {
}
////////////////////////////////////////////////////////////////////////////////////
- // Private implementation details below.
-
- // Constants used to serialize and deserialize
- private static final String PROTO_MESSAGE_CLASS =
"dynamic_proto_message_class";
- private static final String PROTO_EXTENSION_HOSTS =
"dynamic_proto_extension_hosts";
+ // Private implementation details below.∂
Review comment:
nit:
```suggestion
// Private implementation details below.
```
##########
File path:
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingModeExecutionContext.java
##########
@@ -145,18 +145,13 @@ public StreamingModeExecutionContext(
// 2. The reporting thread calls extractUpdate which reads the current sum
*AND* sets it to 0.
private final AtomicLong totalMillisInState = new AtomicLong();
- // The worker that created this state. Used to report lulls back to the
worker.
- private final StreamingDataflowWorker worker;
-
public StreamingModeExecutionState(
NameContext nameContext,
String stateName,
MetricsContainer metricsContainer,
- ProfileScope profileScope,
- StreamingDataflowWorker worker) {
Review comment:
This is also public
--
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]
