Ferenc Szabo 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: (2 comments) Thanks for the reviews! See my comments below. http://gerrit.cloudera.org:8080/#/c/11391/3/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/3/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java@364 PS3, Line 364: default: > Should a runtime exception be thrown if no policy is defined? only a valid ParseErrorPolicy can come out from getParseErrorPolicyCheckingDeprecatedProperty() so this default case is only for checkstyle 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 dupli Doing multiple assertions in one test case would be a bad practice, as you would have to investigate test failures to see which assertion failed. The other reason why I had the ...Deprecated tests is that they can be deleted with the deprecated parameters. As this test class has 4 type of tests, the parametrized version would require a test method that is more complex than what we have now and the parameter array would be long and hard to read. I was considering to make this test parametrized and at the end the way it is now seemed to be the most readable. It is long and has some code repetition in it but the methods are short, simple and have descriptive names. And have only one reason to fail. -- 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 <fsz...@cloudera.com> Gerrit-Reviewer: Ferenc Szabo <fsz...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Tue, 25 Sep 2018 09:27:10 +0000 Gerrit-HasComments: Yes
