Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15191 )
Change subject: [java] Fix various build warnings ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml File java/config/spotbugs/excludeFilter.xml: http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@115 PS3, Line 115: <!-- Often tests testing exceptions have "exception" in the name. --> > Seems easy enough to address (later). The name made sense (PrintStreamWithIOException) so I didn't see a reason to change it. http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@239 PS3, Line 239: <!-- Reference equality is intended here. --> > Not clear how the three <Bug> elements have to do with this. Bad copy paste. http://gerrit.cloudera.org:8080/#/c/15191/3/java/config/spotbugs/excludeFilter.xml@242 PS3, Line 242: <Bug pattern="BIT_ADD_OF_SIGNED_BYTE" /> : <Bug pattern="SF_SWITCH_FALLTHROUGH" /> : <Bug pattern="SF_SWITCH_NO_DEFAULT" /> > You modified the code to disable checkstyle though; does that mean this is This is the spotbugs file. Both tools flagged it. http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java: http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/client/CallResponse.java@161 PS3, Line 161: @Override : public boolean equals(Object o) { : return super.equals(o); : } : : @Override : public int hashCode() { : return super.hashCode(); : } > By default, won't the parent's method be invoked anyways? Yes, but that doesn't satisfy spotbugs. We still want the parent's behavior so I override calling super. http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java: http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-client/src/main/java/org/apache/kudu/util/NetUtil.java@131 PS3, Line 131: new InetAddress[0]; > Yeah I agree this looks fishy. Is it possible for getAllByName() to return I will just undo and add a rule exclusion. http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java: http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/MessageIO.java@96 PS3, Line 96: String.format("unable to receive message, expected (%d) bytes " + : "but read (%d) bytes", size, read)); > Wasn't the earlier formatting correct? Done http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java: http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/ProtocolHandler.java@36 PS3, Line 36: public abstract class ProtocolHandler<I extends Message, : O extends Message> > I find earlier Request/Response generic types better for readability than I I think the issue is that it makes it less clear that they are types and not variables. The checkstlye rule for this is described here: https://checkstyle.sourceforge.io/config_naming.html#ClassTypeParameterName I will do RequestT and ResponseT since that conforms to the rule. http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java File java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java: http://gerrit.cloudera.org:8080/#/c/15191/3/java/kudu-subprocess/src/main/java/org/apache/kudu/subprocess/SubprocessExecutor.java@58 PS3, Line 58: // Exit the program with a nonzero status code, by throwing a runtime exception. : // if unexpected exception(s) are thrown by the reader or writer tasks. > Comment needs to be rewritten. Done -- To view, visit http://gerrit.cloudera.org:8080/15191 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75649e6ad26eec1942811fcb790fc9a2c0866a6b Gerrit-Change-Number: 15191 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 Feb 2020 23:02:08 +0000 Gerrit-HasComments: Yes
