Ferenc Szabo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11334 )
Change subject: KUDU-2012 Kudu Flume sink auth support ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11334/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSinkConfigurationConstants.java File java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSinkConfigurationConstants.java: http://gerrit.cloudera.org:8080/#/c/11334/1/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSinkConfigurationConstants.java@71 PS1, Line 71: public static final String KERBEROS_KEYTAB = "kerberosKeytab"; > Are these Kudu sink specific or can they be imported from somewhere else in Currently, these are just another property of another sink. There is no centralized/modularized version of these configuration constants. So in a way, it is Kudu sink specific, however, it is a repeating concept in flume. http://gerrit.cloudera.org:8080/#/c/11334/1/java/kudu-flume-sink/src/test/java/org/apache/flume/auth/FlumeAuthenticatorUtilTestAdapter.java File java/kudu-flume-sink/src/test/java/org/apache/flume/auth/FlumeAuthenticatorUtilTestAdapter.java: http://gerrit.cloudera.org:8080/#/c/11334/1/java/kudu-flume-sink/src/test/java/org/apache/flume/auth/FlumeAuthenticatorUtilTestAdapter.java@22 PS1, Line 22: * This class is needed because the {@link FlumeAuthenticationUtil#clearCredentials()} > Can a change be made in Flume to change the visibility? Either public or pr Any change on the Flume side would take a long time to release and get back here. I could use reflection to call it from a more convenient test util. Another solution would be to have only one test method in one or two Secure test classes and that way we do not have to reset the FlumeAuthenticationUtil. The basic functionality does not change if it is on a secure cluster so it does not make too much sense anyway. http://gerrit.cloudera.org:8080/#/c/11334/1/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java File java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java: http://gerrit.cloudera.org:8080/#/c/11334/1/java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/KuduSinkTest.java@57 PS1, Line 57: protected KuduTable createNewTable(String tableName) throws Exception { > Perhaps move these to a new test utility class instead of marking them prot I will do a refactor/cleanup with the rest of the fixes http://gerrit.cloudera.org:8080/#/c/11334/1/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/11334/1/src/kudu/mini-cluster/external_mini_cluster.cc@184 PS1, Line 184: RETURN_NOT_OK_PREPEND(kdc_->CreateServiceKeytab("flume-client", &ktpath), > Can we use the test-user instead of a service keytab so this isn't needed? The point of the test is that flume uses a keytab to authenticate. I could use the test-user and create a new method to generate a keytab, as the one we have now changes the password for the test-user and that breaks a few test cases. should I go with the new method (CreateKeytabForExistingPrincipal)? -- To view, visit http://gerrit.cloudera.org:8080/11334 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11b5f08802883afa178d346af48d3bcd15281917 Gerrit-Change-Number: 11334 Gerrit-PatchSet: 1 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-Comment-Date: Mon, 27 Aug 2018 19:11:31 +0000 Gerrit-HasComments: Yes
