Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14801 )
Change subject: [java] Enable and fix low SpotBugs warnings ...................................................................... Patch Set 2: Code-Review+1 (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? 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? It would be nice to add a comment if there is some non-trivial catch about this. 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? 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 return null (per documentation). I'm curious whether the SpotBugs spotted that because of some sort of specific rule for X509TrustManager, NotNull annotation (I could not find one) or that's something generic for methods returning arrays? -- 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: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 27 Nov 2019 08:34:13 +0000 Gerrit-HasComments: Yes
