Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11391 )
Change subject: KUDU-1882 Configuration improvements for Flume Kudu Sink regexp operations producer. ...................................................................... Patch Set 4: (6 comments) Thanks for the patch - looks great, just some nit feedback from me http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java File java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java: http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java@138 PS4, Line 138: attempted s/attempted/produced/ here and elsewhere http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java@148 PS4, Line 148: is still attempted ... is still produced, but does not include the given column http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java@158 PS4, Line 158: operation is still attempted ... and the row is skipped, not producing an operation http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java@159 PS4, Line 159: still attempted the row is skipped (?) http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java@386 PS4, Line 386: policyString .toUpperCase() ? http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerParseErrorTest.java File java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerParseErrorTest.java: http://gerrit.cloudera.org:8080/#/c/11391/4/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerParseErrorTest.java@66 PS4, Line 66: public void testMissingColumnThrowsExceptionDeprecated() throws Exception { : Context additionalContext = new Context(); : additionalContext.put(PATTERN_PROP, TEST_REGEXP_MISSING_COLUMN); : additionalContext.put(SKIP_MISSING_COLUMN_PROP, String.valueOf(false)); : testThrowsException(additionalContext, ERROR_MSG_MISSING_COLUMN, ROW_MISSING_COLUMN); : } : : @Test : public void testMissingColumnThrowsException() throws Exception { : Context additionalContext = new Context(); : additionalContext.put(PATTERN_PROP, TEST_REGEXP_MISSING_COLUMN); : additionalContext.put(MISSING_COLUMN_POLICY_PROP, POLICY_REJECT); : testThrowsException(additionalContext, ERROR_MSG_MISSING_COLUMN, ROW_MISSING_COLUMN); : } nit: did you consider combining these deprecated tests to cut down on duplication? i.e. @Test public void testMissionColumnThrowsException() throws Exception { for (String prop : {String.valueOf(false), POLICY_REJECT}) { Context additionalContext = new Context(); additionalContext.put(PATTERN_PROP, TEST_REGEXP_MISSING_COLUMN); additionalContext.put(MISSING_COLUMN_POLICY_PROP, POLICY_REJECT); testThrowsException(additionalContext, ERROR_MSG_MISSING_COLUMN, ROW_MISSING_COLUMN); } } Alternatively we could parameterize it with JUnit. However I'm not sure if it's straightforward or not, and since you probably already thought of it and decided it wasn't worth the effort, I'm fine with not doing that. Just thought I would ask if you think it's worth doing that. -- To view, visit http://gerrit.cloudera.org:8080/11391 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0b352b27583ae9eee6cdbe28a768362bea36e00f Gerrit-Change-Number: 11391 Gerrit-PatchSet: 4 Gerrit-Owner: Ferenc Szabo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Thu, 20 Sep 2018 23:56:39 +0000 Gerrit-HasComments: Yes
