Grant Henke has posted comments on this change. Change subject: [java] update dependencies and clean up some deprecate method usage ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/7276/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: Line 401: out.writeUInt32NoTag(header.getSerializedSize()); > I think these two lines can be collapsed into one by with 'header.writeDeli 'header.writeDelimitedTo(out);' takes an OutputStream and CodedOutputStream doesn't actually extend OutputStream. http://gerrit.cloudera.org:8080/#/c/7276/3/java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java File java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java: Line 467: throw new RuntimeException(e); > Could you change this method to throw IoException, and then not catch this `handler.getEngine().getSession().getPeerCertificates();` can throw SSLPeerUnverifiedException too. I don't think we want that to throw right? Line 602: throw new RuntimeException(se); > same here SaslException is thrown by `saslClient.unwrap(provided, 4, provided.length - 4);`. Should I catch and convert to an IOException and change the RuntimeExceptions to IOExceptions too? http://gerrit.cloudera.org:8080/#/c/7276/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestFlexiblePartitioning.java: Line 452: public String toString() { > I think you can skip fully qualifying it now that it doesn't conflict with Done http://gerrit.cloudera.org:8080/#/c/7276/3/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableOutputFormat.java File java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableOutputFormat.java: Line 220: LOG.warn("Got per errors for {} rows, the first one being {}", > While you're here could you change this to use slf4j syntax: Done http://gerrit.cloudera.org:8080/#/c/7276/3/java/kudu-spark-tools/pom.xml File java/kudu-spark-tools/pom.xml: Line 129: </dependency> > This got removed, right? Yeah, I did this before I realized it was not used. If that gets in first I will rebase. -- To view, visit http://gerrit.cloudera.org:8080/7276 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b9746f70187c5a7ca0dd016db92ae4e6de7ecc3 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
