Harsh J has posted comments on this change. Change subject: Add Kudu Flume sink. ......................................................................
Patch Set 2: (9 comments) I'm by no means an active reviewer (just a lurker around the lists), so take this with a pinch of salt please! 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 135: "Master name cannot be empty, please specify in configuration file"); Love validations. Can the message also suggest the missing config name inline (same for the table name)? Line 169: return Status.READY; Not a flume expert, but do we want BACKOFF here? Line 206: throw new EventDeliveryException("Failed to flush one or more changes." + Missing space Line 211: throw new EventDeliveryException("Failed to flush changes." + Missing space Line 239: String msg = "Failed to commit transaction. Transaction rolled back."; This could be a little confusing. If the rollback fails above this point, we'd log "might not have been successful" AND "Transaction rolled back" both, misleading as to what may really have happened (to a person reading logs). Certainly we'd want both messages (of why the rollback occurred in the first place, and of why the rollback failed when attempted), but the logging should be consistent with the "might not have been successful" sort of message with both reasons captured within. Line 241: if(e instanceof Error || e instanceof RuntimeException){ Would catching EventDeliveryException specifically in the catch clause above (L231) be better than doing these checks for re-throw? http://gerrit.cloudera.org:8080/#/c/2600/2/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 28: public static final String CONFIG_MASTER_ADDRESS = "master"; Just a personal nit: Since this is already a Configuration named class, the vars can likely have their CONFIG_ prefixes removed. http://gerrit.cloudera.org:8080/#/c/2600/2/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 75: throw new FlumeException("Could not get row key!", e); This error message may appear vague without context… Does it just refer to the table.newInsert() failure? 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 exception is 'try { something(); Assert.fail("something should not have passed cause…"); } catch (Exception e) { }'. The fail statement would fail the test if the exception does not get thrown in future. -- 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
