Mike Percy has posted comments on this change.

Change subject: Add AvroKuduEventProducer to Kudu-Flume integration
......................................................................


Patch Set 6:

(7 comments)

Found a few more test nits. I think this is nearly ready to commit now.

http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/pom.xml
File java/kudu-flume-sink/pom.xml:

Line 82:                 
<sourceDirectory>${project.basedir}/src/main/avro/</sourceDirectory>
this should not be in src/main it should be in src/test since this is only used 
in the test module.

In fact, that is the default for the generate-test-sources goal and you should 
be able to leave off the configuration section completely from here. I just 
tested this and it's true, you can remove this section.


Line 85:               </execution>
nit: this line should be un-indented by 2 spaces


http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc
File java/kudu-flume-sink/src/test/avro/testAvroEventProducer.avsc:

Line 5:     {"name": "key", "type": "int"},
I don't think we should name these fields with names that match types exactly. 
It makes the generated Avro method names a bit weird, and people reading the 
tests might be a little confused.

Instread of "long", how about 
"longField" or something? Instead of "null", how about "nullableStringField"?

Suggestions:

* key (this is alright, or intKey)
* longField
* doubleField
* nullableStringField
* stringField


http://gerrit.cloudera.org:8080/#/c/4034/6/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java
File 
java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduEventProducerTest.java:

Line 136:       assertTrue("incorrect status for empty channel", status == 
Sink.Status.BACKOFF);
assertEquals("incorrect status for empty channel", status, Sink.Status.BACKOFF);


Line 138:       assertTrue("incorrect status for non-empty channel", status != 
Sink.Status.BACKOFF);
assertEquals("incorrect status for non-empty channel", status, 
Sink.Status.READY);


Line 148:     ArrayList<ColumnSchema> columns = new ArrayList<>(3);
nit: initial capacity should be 5


Line 179:       record.setLong$(2L * i);
See my comments about this $ syntax in the .avsc file -- let's just rename 
these fields not to conflict with type names.


-- 
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: 6
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

Reply via email to