You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/08/09 13:35:28 UTC

[GitHub] [accumulo] EdColeman opened a new pull request, #2861: Fix additional spotbugs errors in core

EdColeman opened a new pull request, #2861:
URL: https://github.com/apache/accumulo/pull/2861

   Fix spotbugs errors identified by temporarily setting the spotbugs effort maxRank at 17 (normally 16).  In core, there are 35 errors identified - this PR address 17 of them.
   
   <details>
     <summary>Fixed errors - Click to expand!</summary>
     
     - [ERROR] Medium: Hard coded reference to an absolute pathname in org.apache.accumulo.core.client.ClientPropertiesTest.testBasic() [org.apache.accumulo.core.client.ClientPropertiesTest] At ClientPropertiesTest.java:[line 45] DMI_HARDCODED_ABSOLUTE_FILENAME
     - [ERROR] Medium: Return value of java.util.regex.Pattern.matcher(CharSequence) ignored, but method has no side effect [org.apache.accumulo.core.iterators.user.RegExFilter, org.apache.accumulo.core.iterators.user.RegExFilter, org.apache.accumulo.core.iterators.user.RegExFilter, org.apache.accumulo.core.iterators.user.RegExFilter] At RegExFilter.java:[line 178]Another occurrence at RegExFilter.java:[line 181]Another occurrence at RegExFilter.java:[line 184]Another occurrence at RegExFilter.java:[line 187] RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT
     - [ERROR] Medium: Dead store to more in org.apache.accumulo.core.metadata.MetadataLocationObtainer.lookupTablet(ClientContext, TabletLocator$TabletLocation, Text, Text, TabletLocator) [org.apache.accumulo.core.metadata.MetadataLocationObtainer] At MetadataLocationObtainer.java:[line 118] DLS_DEAD_LOCAL_STORE
     - [ERROR] Medium: Write to static field org.apache.accumulo.core.spi.balancer.TableLoadBalancerTest.state from instance method org.apache.accumulo.core.spi.balancer.TableLoadBalancerTest.test() [org.apache.accumulo.core.spi.balancer.TableLoadBalancerTest] At TableLoadBalancerTest.java:[line 136] ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD
     - [ERROR] Medium: Integral division result cast to double or float in org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest.testEvenWeightsNoCaching() [org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest, org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest] At SpaceAwareVolumeChooserTest.java:[line 120]Another occurrence at SpaceAwareVolumeChooserTest.java:[line 121] ICAST_IDIV_CAST_TO_DOUBLE
     - [ERROR] Medium: Integral division result cast to double or float in org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest.testEvenWeightsWithCaching() [org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest, org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest] At SpaceAwareVolumeChooserTest.java:[line 108]Another occurrence at SpaceAwareVolumeChooserTest.java:[line 109] ICAST_IDIV_CAST_TO_DOUBLE
     - [ERROR] Medium: Integral division result cast to double or float in org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest.testNinetyTen() [org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest, org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest] At SpaceAwareVolumeChooserTest.java:[line 138]Another occurrence at SpaceAwareVolumeChooserTest.java:[line 139] ICAST_IDIV_CAST_TO_DOUBLE
     - [ERROR] Medium: Integral division result cast to double or float in org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest.testTenNinety() [org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest, org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest] At SpaceAwareVolumeChooserTest.java:[line 150]Another occurrence at SpaceAwareVolumeChooserTest.java:[line 151] ICAST_IDIV_CAST_TO_DOUBLE
     - [ERROR] Medium: Integral division result cast to double or float in org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest.testWithNoCaching() [org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest, org.apache.accumulo.core.spi.fs.SpaceAwareVolumeChooserTest] At SpaceAwareVolumeChooserTest.java:[line 162]Another occurrence at SpaceAwareVolumeChooserTest.java:[line 163] ICAST_IDIV_CAST_TO_DOUBLE
     - [ERROR] Medium: org.apache.accumulo.core.util.LocalityGroupUtil$PartitionedMutation.equals(Object) is unusual [org.apache.accumulo.core.util.LocalityGroupUtil$PartitionedMutation] At LocalityGroupUtil.java:[line 278] EQ_UNUSUAL
     - [ERROR] Medium: Return value of PreAllocatedArray.get(int) ignored, but method has no side effect [org.apache.accumulo.core.util.PreAllocatedArrayTest] At PreAllocatedArrayTest.java:[line 101] RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT
     - [ERROR] Medium: Return value of PreAllocatedArray.get(int) ignored, but method has no side effect [org.apache.accumulo.core.util.PreAllocatedArrayTest] At PreAllocatedArrayTest.java:[line 108] RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT
     - [ERROR] Medium: Return value of PreAllocatedArray.get(int) ignored, but method has no side effect [org.apache.accumulo.core.util.PreAllocatedArrayTest] At PreAllocatedArrayTest.java:[line 100] RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT
     - [ERROR] Medium: Return value of PreAllocatedArray.get(int) ignored, but method has no side effect [org.apache.accumulo.core.util.PreAllocatedArrayTest] At PreAllocatedArrayTest.java:[line 107] RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT
     - [ERROR] Medium: Exception is caught when Exception is not thrown in org.apache.accumulo.fate.ZooStore.reserve() [org.apache.accumulo.fate.ZooStore] At ZooStore.java:[line 211] REC_CATCH_EXCEPTION
     - [ERROR] Medium: Return value of Retry.canRetry() ignored, but method has no side effect [org.apache.accumulo.fate.util.RetryTest] At RetryTest.java:[line 84] RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT
   
   </details>


-- 
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: notifications-unsubscribe@accumulo.apache.org

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

[GitHub] [accumulo] EdColeman commented on a diff in pull request #2861: Fix additional spotbugs errors in core

Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2861:
URL: https://github.com/apache/accumulo/pull/2861#discussion_r944070110


##########
core/src/main/java/org/apache/accumulo/core/metadata/MetadataLocationObtainer.java:
##########
@@ -115,9 +115,9 @@ public TabletLocations lookupTablet(ClientContext context, TabletLocation src, T
         range = new Range(results.lastKey().followingKey(PartialKey.ROW_COLFAM_COLQUAL_COLVIS_TIME),
             true, new Key(stopRow).followingKey(PartialKey.ROW), false);
         encodedResults.clear();
-        more = ThriftScanner.getBatchFromServer(context, range, src.tablet_extent,
-            src.tablet_location, encodedResults, locCols, serverSideIteratorList,
-            serverSideIteratorOptions, Constants.SCAN_BATCH_SIZE, Authorizations.EMPTY, 0L, null);
+        ThriftScanner.getBatchFromServer(context, range, src.tablet_extent, src.tablet_location,
+            encodedResults, locCols, serverSideIteratorList, serverSideIteratorOptions,
+            Constants.SCAN_BATCH_SIZE, Authorizations.EMPTY, 0L, null);

Review Comment:
   I don't recall at the moment - but I likely just took the IDE suggested change.  If you believe this is incorrect, please feel to revert this change and we can revisit it in isolation.



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2861: Fix additional spotbugs errors in core

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2861:
URL: https://github.com/apache/accumulo/pull/2861#discussion_r1016193609


##########
core/src/main/java/org/apache/accumulo/core/metadata/MetadataLocationObtainer.java:
##########
@@ -115,9 +115,9 @@ public TabletLocations lookupTablet(ClientContext context, TabletLocation src, T
         range = new Range(results.lastKey().followingKey(PartialKey.ROW_COLFAM_COLQUAL_COLVIS_TIME),
             true, new Key(stopRow).followingKey(PartialKey.ROW), false);
         encodedResults.clear();
-        more = ThriftScanner.getBatchFromServer(context, range, src.tablet_extent,
-            src.tablet_location, encodedResults, locCols, serverSideIteratorList,
-            serverSideIteratorOptions, Constants.SCAN_BATCH_SIZE, Authorizations.EMPTY, 0L, null);
+        ThriftScanner.getBatchFromServer(context, range, src.tablet_extent, src.tablet_location,
+            encodedResults, locCols, serverSideIteratorList, serverSideIteratorOptions,
+            Constants.SCAN_BATCH_SIZE, Authorizations.EMPTY, 0L, null);

Review Comment:
   Getting rid of the extra assignment is a valid IDE suggestion, since it's never used again. However, it looks like it was supposed to be used. There may yet still be a bug here, but the code is not very well documented or intuitive. I created #3072 to investigate this.



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2861: Fix additional spotbugs errors in core

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2861:
URL: https://github.com/apache/accumulo/pull/2861#discussion_r943857845


##########
core/src/main/java/org/apache/accumulo/core/metadata/MetadataLocationObtainer.java:
##########
@@ -115,9 +115,9 @@ public TabletLocations lookupTablet(ClientContext context, TabletLocation src, T
         range = new Range(results.lastKey().followingKey(PartialKey.ROW_COLFAM_COLQUAL_COLVIS_TIME),
             true, new Key(stopRow).followingKey(PartialKey.ROW), false);
         encodedResults.clear();
-        more = ThriftScanner.getBatchFromServer(context, range, src.tablet_extent,
-            src.tablet_location, encodedResults, locCols, serverSideIteratorList,
-            serverSideIteratorOptions, Constants.SCAN_BATCH_SIZE, Authorizations.EMPTY, 0L, null);
+        ThriftScanner.getBatchFromServer(context, range, src.tablet_extent, src.tablet_location,
+            encodedResults, locCols, serverSideIteratorList, serverSideIteratorOptions,
+            Constants.SCAN_BATCH_SIZE, Authorizations.EMPTY, 0L, null);

Review Comment:
   This doesn't look correct. Should this `if` loop be a `while` loop instead, so the `more` field is read? What happens if `more` is true here?



##########
core/src/test/java/org/apache/accumulo/fate/util/RetryTest.java:
##########
@@ -81,7 +81,7 @@ public void retriesAreCompleted() {
     for (int i = 0; i < MAX_RETRIES; i++) {
       assertEquals(i, retry.retriesCompleted());
       // canRetry doesn't alter retry's state
-      retry.canRetry();
+      assertTrue(retry.canRetry());

Review Comment:
   Should probably `assertFalse(retry.canRetry())` after the loop.



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2861: Fix additional spotbugs errors in core

Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2861:
URL: https://github.com/apache/accumulo/pull/2861#discussion_r1016184363


##########
core/src/test/java/org/apache/accumulo/fate/util/RetryTest.java:
##########
@@ -81,7 +81,7 @@ public void retriesAreCompleted() {
     for (int i = 0; i < MAX_RETRIES; i++) {
       assertEquals(i, retry.retriesCompleted());
       // canRetry doesn't alter retry's state
-      retry.canRetry();
+      assertTrue(retry.canRetry());

Review Comment:
   Nevermind. Next test covers this case already.



-- 
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: notifications-unsubscribe@accumulo.apache.org

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

[GitHub] [accumulo] EdColeman merged pull request #2861: Fix additional spotbugs errors in core

Posted by GitBox <gi...@apache.org>.
EdColeman merged PR #2861:
URL: https://github.com/apache/accumulo/pull/2861


-- 
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: notifications-unsubscribe@accumulo.apache.org

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