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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
