Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9050 )
Change subject: WIP: KUDU-2264. Automatically attempt to re-acquire Kerberos credentials before expiration ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java File java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java: http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java@90 PS1, Line 90: private Object subjectLock = new Object(); add 'final' http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java: http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java@78 PS1, Line 78: logins 'a new subject is logged in' http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java@186 PS1, Line 186: if (principal.getName().equals("krbtgt/" + principal.getRealm() + simplify this to return principal.getName().equals("krbtgt/" + principal.getRealm() + "@" + principal.getRealm()) http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@107 PS1, Line 107: Thread.sleep(5000); Could you add an assert which guarantees the if branch is actually taken? Right now the test is a bit brittle to the TICKED_LIFETIME_SECS value changing in the other file, since the number of loops and sleep time is constant. http://gerrit.cloudera.org:8080/#/c/9050/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@128 PS1, Line 128: // TODO(todd) add test which externally provides a subject and ensures that it doesn't I guess one downside of removing the Java MiniKdc is that this becomes much harder. http://gerrit.cloudera.org:8080/#/c/9050/1/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/9050/1/src/kudu/tools/tool.proto@157 PS1, Line 157: // is kinitted by default when the cluster starts. Instead of documenting this, consider adding it as the protobuf field default: optional string usernam = 1 [default = "test-admin"]; http://gerrit.cloudera.org:8080/#/c/9050/1/src/kudu/tools/tool_action_test.cc File src/kudu/tools/tool_action_test.cc: http://gerrit.cloudera.org:8080/#/c/9050/1/src/kudu/tools/tool_action_test.cc@288 PS1, Line 288: string username = req.kinit().has_username() ? req.kinit().username() : "test-admin"; This dance becomes unnecessary with the default defined in the .proto. -- To view, visit http://gerrit.cloudera.org:8080/9050 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I514253e0a7f067dbc8ffe4eaf5a7a2c32900b539 Gerrit-Change-Number: 9050 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Thu, 18 Jan 2018 19:06:49 +0000 Gerrit-HasComments: Yes
