Jean-Daniel Cryans has posted comments on this change. Change subject: Add Kudu Flume sink. ......................................................................
Patch Set 14: (14 comments) Lots of nits but really need to fix the license header in the unit test. http://gerrit.cloudera.org:8080/#/c/2600/14/java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/KuduEventProducer.java File java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/KuduEventProducer.java: Line 35: public interface KuduEventProducer extends Configurable, ConfigurableComponent { This interface should be marked InterfaceAudience.Public and InterfaceStability.Evolving Line 38: . nit, remove Line 47: . nit http://gerrit.cloudera.org:8080/#/c/2600/14/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 48: parameters This is plural, is there supposed to be more than one? Line 51: public class KuduSink extends AbstractSink implements Configurable { This whole class should be marked InterfaceAudience.Public and InterfaceStability.Evolving Line 129: public void configure(Context context) { nit: this whole method needs more spacing. Line 141: name hostnames? addresses? Line 148: e nit: inline comments end with a period. Line 151: W nit: w http://gerrit.cloudera.org:8080/#/c/2600/14/java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/KuduSinkConfigurationConstants.java File java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/KuduSinkConfigurationConstants.java: Line 24: public class KuduSinkConfigurationConstants { This interface should be marked InterfaceAudience.Public and InterfaceStability.Evolving Line 26: * The Kudu master host address. There could be many. Line 46: Why is this spaced and not the rest? http://gerrit.cloudera.org:8080/#/c/2600/14/java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/SimpleKuduEventProducer.java File java/kudu-flume-sink/src/main/java/org/kududb/flume/sink/SimpleKuduEventProducer.java: Line 41: payload will be assumed Will that ever be useful? Or I guess this whole class is more an example than something people should be using. http://gerrit.cloudera.org:8080/#/c/2600/14/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 1: package org.kududb.flume.sink; License -- 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: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Ara Ebrahimi <[email protected]> Gerrit-Reviewer: Harsh J <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-HasComments: Yes
