Will Berkeley has posted comments on this change. Change subject: API and style improvements to the Kudu Flume Sink ......................................................................
Patch Set 1: (8 comments) Do we also want something in the release notes about the API change + AvroKuduOperationsProducer? 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 Done 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 Done 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 101: private KuduOperationsProducer eventProducer; > Shouldn't this also be called operationsProducer instead of eventProducer? Done Line 145: eventProducer.close(); > I think we should wrap this in a try and log if it throws, then continue. Done Line 154: throw new FlumeException("Error closing client", e); > I think we should log errors but continue with shutdown here. Otherwise it Done Line 158: } > I'd suggest throwing at the very end if we got any exceptions while shuttin Done Line 165: String.format("Missing master addresses. Please specify property '$s'.", > nit: here and elsewhere, checkNotNull() supports substitution syntax: https Done 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: Done -- 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-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
