[GitHub] [beam] TheNeuralBit commented on a change in pull request #15890: [BEAM-11936] Fix errorprone warnings

[GitBox <gi...@apache.org>]

TheNeuralBit commented on a change in pull request #15890:
URL: https://github.com/apache/beam/pull/15890#discussion_r762206314



##########
File path: examples/java/src/main/java/org/apache/beam/examples/snippets/Snippets.java
##########
@@ -191,6 +191,7 @@ public static void modelBigQueryIO(
 
     {
       // [START BigQueryDataTypes]
+      @SuppressWarnings("ModifiedButNotUsed")

Review comment:
       Is there a way to suppress this so it's not in between the START and END comments? Everything there gets pulled into documentation on beam.apache.org

##########
File path: runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/DataflowElementExecutionTracker.java
##########
@@ -311,6 +311,7 @@ private void attributeProcessingTime(Duration duration, long snapshot) {
             executionCount.getKey(),
             (unused, total) -> {
               int numExecutions = executionCount.getValue();
+              @SuppressWarnings("PreferJavaTimeOverload")

Review comment:
       Is this a false positive? I'm not sure what the check is expecting you to change here

##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/kafka/BeamKafkaTable.java
##########
@@ -222,6 +222,7 @@ public BeamTableStatistics getTableStatistics(PipelineOptions options) {
       throw new NoEstimationException("There is no partition with messages in it.");
     }
 
+    @SuppressWarnings("PreferJavaTimeOverload")
     ConsumerRecords<T, T> records = consumer.poll(1000);

Review comment:
       ```suggestion
       ConsumerRecords<T, T> records = consumer.poll(Duration.ofMillis(1000));
   ```

##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/splittabledofn/GrowableOffsetRangeTrackerTest.java
##########
@@ -183,7 +183,7 @@ public void testProgressAfterFinished() throws Exception {
     tracker.checkDone();
     simpleEstimator.setEstimateRangeEnd(0L);
     Progress currentProgress = tracker.getProgress();
-    assertEquals(Long.MAX_VALUE - 10L, currentProgress.getWorkCompleted(), 0.001);
+    assertEquals(Long.MAX_VALUE - 10L, currentProgress.getWorkCompleted(), 0);

Review comment:
       I think the spirit of https://errorprone.info/bugpattern/FloatingPointAssertionWithinEpsilon is that we shouldn't specify epsilon, since it's small enough that we're doing an exact equality check anyway.
   ```suggestion
       assertEquals(Long.MAX_VALUE - 10L, currentProgress.getWorkCompleted();
   ```

##########
File path: sdks/java/harness/src/test/java/org/apache/beam/fn/harness/state/StateFetchingIteratorsTest.java
##########
@@ -268,7 +267,7 @@ public void testManyValues() throws Exception {
           assertEquals(Math.min(i + 2, expected.length), callCount.get());
         }
         assertTrue(valuesIter2.hasNext());
-        results.add(valuesIter2.next());
+        valuesIter2.next();
       }
       assertFalse(valuesIter2.hasNext());
       assertTrue(valuesIter2.isReady());

Review comment:
       Can you try adding that? If it makes the test fail let's file a jira to follow up on this.

##########
File path: sdks/java/harness/src/test/java/org/apache/beam/fn/harness/state/StateFetchingIteratorsTest.java
##########
@@ -268,7 +267,7 @@ public void testManyValues() throws Exception {
           assertEquals(Math.min(i + 2, expected.length), callCount.get());
         }
         assertTrue(valuesIter2.hasNext());
-        results.add(valuesIter2.next());
+        valuesIter2.next();
       }
       assertFalse(valuesIter2.hasNext());
       assertTrue(valuesIter2.isReady());

Review comment:
       I asked about this over in https://github.com/apache/beam/pull/15475/files/0ebe6567a2057cdb642833c5c9d28853ec5ecdd6#r762215539
   
   It seems the intention was to validate `results` like this:
   ```suggestion
         assertTrue(valuesIter2.isReady());
         assertArrayEquals(expected, Iterables.toArray(results, Object.class));
   ```




-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org