Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14747 )
Change subject: [java] Fix medium SpotBugs issues ...................................................................... Patch Set 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml File java/config/spotbugs/excludeFilter.xml: PS1: For the PB stuff, there are a few classes that lack a PB suffix. Could we rename them in the code while preserving backwards compatibility? http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@28 PS1, Line 28: <Class name="~org\.apache\.kudu\.consensus\.Opid.*"/> Don't need a .* at the end; it's literally just the class built for the OpId message. http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@29 PS1, Line 29: <Class name="~org\.apache\.kudu\.master\.Master.*"/> So this is for the "service endpoint" generated code, right? What about the tablet service endpoints? http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@30 PS1, Line 30: <Class name="~org\.apache\.kudu\.rpc\.RpcHeader.*"/> You could restrict this to just RequestHeader and ResponseHeader; every other top-level message in rpc_header.proto is suffixed with a PB. http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@79 PS1, Line 79: <!-- The retry rule doesn't need to be read. --> We should add guidance here (or maybe in RetryRule itself) to use a well-known naming scheme for RetryRule instantiations to avoid running afoul of SpotBugs. http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@86 PS1, Line 86: TestKuduTable Sentence fragment? http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@96 PS1, Line 96: <!-- Tests use i % 2 == 1 frequently to alternate behavior. --> : <Class name="~org\.apache\.kudu.*Test.*"/> : <Bug pattern="IM_BAD_CHECK_FOR_ODD" /> Any interest in converting these into negated "check for even" instead? Could be done in a follow-up. http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@103 PS1, Line 103: <Class name="org.apache.kudu.backup.KuduBackupRDD"/> : <Field name="options" /> : <Bug pattern="SE_TRANSIENT_FIELD_NOT_RESTORED" /> Anything worth documenting for this case? http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@116 PS1, Line 116: the they http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@125 PS1, Line 125: <!-- The nullable annotation is from Guava and therefore can't be changed. --> I don't see these annotations in TestFlexiblePartitioning.java. What am I looking for? http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@151 PS1, Line 151: <Class name="org.apache.kudu.spark.kudu.KuduRDD"/> : <Or> : <Field name="options" /> : <Field name="projectedCols"/> : </Or> : <Bug pattern="SE_TRANSIENT_FIELD_NOT_RESTORED" /> Anything worth documenting? http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@161 PS1, Line 161: <!-- The Random is called in the super constructor and can't be re-used. --> : <Or> : <Class name="org.apache.kudu.test.CapturingLogAppender"/> : <Class name="org.apache.kudu.test.CapturingToFileLogAppender"/> : </Or> : <Bug pattern="DMI_RANDOM_USED_ONLY_ONCE" /> FWIW, we could use a static AtomicInt counter here instead of a one-shot Random. Or even a static Random. http://gerrit.cloudera.org:8080/#/c/14747/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java: http://gerrit.cloudera.org:8080/#/c/14747/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPartitioner.java@173 PS1, Line 173: return Objects.hashCode((Object) bytes); Is this preferred to Arrays.hashCode(bytes)? http://gerrit.cloudera.org:8080/#/c/14747/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java File java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java: http://gerrit.cloudera.org:8080/#/c/14747/2/java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java@60 PS2, Line 60: Boolean isLocal = isLocalAddressCache.get(resolvedAddr); : if (isLocal == null) { : isLocal = NetUtil.isLocalAddress(resolvedAddr); : isLocalAddressCache.putIfAbsent(resolvedAddr, isLocal); : } computeIfAbsent is probably a clearer way to do this. http://gerrit.cloudera.org:8080/#/c/14747/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java: http://gerrit.cloudera.org:8080/#/c/14747/2/java/kudu-test-utils/src/main/java/org/apache/kudu/test/ClientTestUtil.java@174 PS2, Line 174: thread.start(); So previously the count was actually being invoked serially? Nice. -- To view, visit http://gerrit.cloudera.org:8080/14747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f66176dcca9dbf77853b54bef20d947f3732e3f Gerrit-Change-Number: 14747 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 19 Nov 2019 23:50:18 +0000 Gerrit-HasComments: Yes
