Mike Percy has posted comments on this change. Change subject: Add Kudu Flume sink. ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/2600/2/java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/KuduSink.java File java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/KuduSink.java: Line 56: private static final String DEFAULT_KUDU_EVENT_PRODUCER = "org.apache.flume.sink.kudu.SimpleKuduEventProducer"; style: Line goes beyond 100 chars. Line 135: "Master name cannot be empty, please specify in configuration file"); > Love validations. Can the message also suggest the missing config name inli +1 Line 165: //if for whatever reason we have pending operations then just refuse to process Style nit: follow the Google style guide for comments, which says to capitalize sentences and leave a space after the "//". Like this: // If for whatever reason we have pending operations then just refuse to process // and tell caller to try again a bit later. We don't want to pile on the Kudu // session object. Line 169: return Status.READY; > Not a flume expert, but do we want BACKOFF here? Yes, should be BACKOFF, otherwise this will spin-loop. Line 199: //only throw an EventDeliveryException if at least one of the responses was Style: spacing and capitalized sentences. Line 232: try{ nit: missing space after try Line 233: txn.rollback(); You should not need to wrap rollback() in a try block. It should never throw unless there is something very seriously wrong, in which case there isn't really anything reasonable we can do. Here is a good model for the idiom: https://github.com/apache/flume/blob/trunk/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java#L457 http://gerrit.cloudera.org:8080/#/c/2600/2/java/kudu-flume-sink/src/test/java/org/kududb/flume/sink/KuduSinkTest.java File java/kudu-flume-sink/src/test/java/org/kududb/flume/sink/KuduSinkTest.java: Line 49: Configurables.configure(sink, context); > Just a general observance here: The test pattern when expecting an exceptio +1 on this -- To view, visit http://gerrit.cloudera.org:8080/2600 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53e02580908ba2468b216543719ebe5011a267c3 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ara Ebrahimi <[email protected]> Gerrit-Reviewer: Harsh J <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-HasComments: Yes
