Will Berkeley has posted comments on this change. Change subject: Add AvroKuduEventProducer to Kudu-Flume integration ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/4034/1/java/kudu-flume-sink/pom.xml File java/kudu-flume-sink/pom.xml: Line 74: <artifactId>hadoop-client</artifactId> > is this typical for Flume sinks? does this get shaded? Or would we expect i To be honest, I wasn't sure about this, so I copied off of the map/reduce integration's pom. I think we do expect it to be on the classpath. The dependency is only needed because I'd like users to be able to load the schema from HDFS, not just the local filesystem. http://gerrit.cloudera.org:8080/#/c/4034/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java File java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduEventProducer.java: Line 51: * <tr><td>producer.upsert</td><td>false</td><td>No</td><td>Whether to upsert or insert rows into Kudu.</td></tr> > - can you wrap these to multiple lines for better readability? Done PS1, Line 52: producer.schema > this sounds like it would be a schema rather than a path. perhaps schema_pa Done Line 80: FileSystem fs = FileSystem.get(path.toUri(), new Configuration()); > I think this should be path.getFileSystem(new Configuration()) Done Line 93: public void initialize(Event event, KuduTable table) { > unrelated suggestion: this is a very strange method name to me, since it ge Yup, it's weird. Line 126: String.format("Column %s is not nullable but was decoded as null", name)); > does this happen if you have a missing column? if so, I think a fallback to Done Line 133: row.addBoolean(name, (boolean) value); > all of these could generate ClassCastExceptions, right? Do we want to do an We could improve the API, but it would mean changing KuduSink and the KuduEventProducer interface, and therefore the other event producers. I don't think the user-facing configuration would need to change. Assuming I'm right, separate patch or broaden the scope of this one? -- To view, visit http://gerrit.cloudera.org:8080/4034 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6715df72e447e72f4801a2e026f6840d09b401e1 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
