Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/14747 )
Change subject: [java] Fix medium SpotBugs issues ...................................................................... Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/14747/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14747/2//COMMIT_MSG@7 PS2, Line 7: SpotBugs > It would be nice to add information README.md on how to run the checks (thi This is a bug from the previous/parent patch, I will fix it there. I will update the readme in a follow up change. For now, `check` works, but you could also run `gradle spotBugs` to just run the spotBugs checks. `gradle tasks` will show all the available common tasks. 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 r That might be possible. Could it be follow on work? 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 OpI The actual issue is reported on `Opid$OpId$Builder` 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 I only included the non PB classes that reported issues instead of covering all of them. 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 oth I was okay being heavy handed given the ideal outcome is ignoring all Protobuf source. 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-kn I will add a note here. It should be obvious to avoid going forward once the pre-commit breaks on SpotBugs issues. http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@86 PS1, Line 86: TestKuduTable > Sentence fragment? oops 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? Will add a TODO for 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? Done http://gerrit.cloudera.org:8080/#/c/14747/1/java/config/spotbugs/excludeFilter.xml@116 PS1, Line 116: the > they Done 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 l It's from the guava Predicate implementations. Guava is adding @Nullable on the parent apply method and that's being detected here for some reason. I will be more specific. 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? Done 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 Ra Done 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)? Done -- 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: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 20 Nov 2019 03:48:56 +0000 Gerrit-HasComments: Yes
