Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java File java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java: http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java@739 PS2, Line 739: context.write(new Text(keyString), new Text(refsList.toString())); > Isn't it a functional change? Does test pass after this has been changed? No, refsList is guaranteed not to be null because it's initialized just above at line 716. http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java File java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java: http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java@69 PS2, Line 69: transient > Why is this important? Is this exception is ever serialized? Exceptions are marked as serializable, but these fields aren't serialized. We don't really care about serializing this exception, so I mark them as transient. http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java: http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@116 PS2, Line 116: if (!hasNext()) { : throw new NoSuchElementException(); : } > Good catch! It seems we don't have much test coverage for this method? We have decent coverage, but always called hasNext() before next() or used a forEach loop. http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java: http://gerrit.cloudera.org:8080/#/c/14801/2/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@437 PS2, Line 437: X509Certificate[0]; > Interesting: I guess this method is not used at all, but it should not retu SpotBugs reported "PZLA_PREFER_ZERO_LENGTH_ARRAYS: Consider returning a zero length array rather than null". Preferring empty collections over null is somewhat standard in Java. I checked the Javadoc to verify the expectation and made the change. -- To view, visit http://gerrit.cloudera.org:8080/14801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4e5c8285554ff84e948c0ea095ba476969377440 Gerrit-Change-Number: 14801 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 27 Nov 2019 17:13:42 +0000 Gerrit-HasComments: Yes
