[GitHub] [accumulo] EdColeman commented on a diff in pull request #3063: test improvements from PR #1333 and check-style clean up

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

EdColeman commented on code in PR #3063:
URL: https://github.com/apache/accumulo/pull/3063#discussion_r1011773923


##########
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/lru/CachedBlockQueue.java:
##########
@@ -73,7 +73,7 @@ public void add(CachedBlock cb) {
       heapSize += cb.heapSize();
     } else {
       CachedBlock head = queue.peek();
-      if (cb.compareTo(head) > 0) {
+      if (head != null && cb.compareTo(head) > 0) {

Review Comment:
   I targeted 2.1.1 because this address issues with the test where they were not testing what was expected - the block size calculations were being ignored. While the function correctly, they were not tested by these tests.  If a future bug fix was identified in 2.1 and implemented, then the updated tests might help so the changes were functioning as expected.
   
   The additional of the null check was to resolve check style identifying that head could be null and cause an NPE. And while it might not occur with the current code, it could in the future.  The other options would be to modify the compareTo to properly handle nulls or use a NonNull annotation (probably preferred) but those seemed more intrusive.



-- 
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