Mike Percy has posted comments on this change. Change subject: API and style improvements to the Kudu Flume Sink ......................................................................
Patch Set 1: (7 comments) Looks like good changes, and if we are going to break the API then we should do it before Kudu 1.0 http://gerrit.cloudera.org:8080/#/c/4320/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducer.java File java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducer.java: Line 243: String.format("Failed to coerce value for column `%s` to type %s", nit: if we are going to quote, let's use single-quotes, not backquotes here. backquotes make me think of commands executed in bash http://gerrit.cloudera.org:8080/#/c/4320/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduOperationsProducer.java File java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduOperationsProducer.java: Line 36: public interface KuduOperationsProducer extends Configurable { How about also: extends AutoCloseable http://gerrit.cloudera.org:8080/#/c/4320/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSink.java File java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSink.java: Line 145: eventProducer.close(); I think we should wrap this in a try and log if it throws, then continue. Line 154: throw new FlumeException("Error closing client", e); I think we should log errors but continue with shutdown here. Otherwise it will screw up the preconditions in start(). Line 158: } I'd suggest throwing at the very end if we got any exceptions while shutting down. Line 165: String.format("Missing master addresses. Please specify property '$s'.", nit: here and elsewhere, checkNotNull() supports substitution syntax: https://google.github.io/guava/releases/snapshot/api/docs/com/google/common/base/Preconditions.html#checkNotNull(T,%20java.lang.String,%20java.lang.Object...) e.g.: Preconditions.checkNotNull(masterAddresses, "Missing master addresses. Please specify property '%s'.", MASTER_ADDRESSES); http://gerrit.cloudera.org:8080/#/c/4320/1/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java File java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KeyedKuduOperationsProducerTest.java: Line 209: parameters.put(KuduSinkConfigurationConstants.PRODUCER, "org.apache.kudu.flume.sink.SimpleKeyedKuduOperationsProducer"); nit: please wrap this long line. also prefer: parameters.put(KuduSinkConfigurationConstants.PRODUCER, SimpleKeyedKuduOperationsProducer.class.getName()); If you do that when possible, IDE refactorings and shading have a better chance of working. -- To view, visit http://gerrit.cloudera.org:8080/4320 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I357df8cac7daa6ce105f9568cc3af09697032eb6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Ara Ebrahimi <ara.ebrah...@argyledata.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-HasComments: Yes
