Adar Dembo has posted comments on this change. Change subject: [java-client]: support for Kerberized RPC ......................................................................
Patch Set 1: (14 comments) I only really looked at the test changes, so don't consider this a full review (I'm deferring to the other reviewers for that). http://gerrit.cloudera.org:8080/#/c/5150/1//COMMIT_MSG Commit Message: PS1, Line 10: be redundant http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: Line 950: * The subject contains credentials necessary to Kerberized Kudu clusters. Nit: "to connect to Kerberized..." maybe? http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java: PS1, Line 228: createKrb5Conf Would it be useful to extract this and kdc.conf into template files that are shared by both mini cluster implementations, with substitutions happening at runtime? Line 333: .redirectInput(ProcessBuilder.Redirect.PIPE) If you've removed redirectOutput() because PIPE is the default value, you can also remove redirectInput(). http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java: Line 107 Technically this is still true... Line 111 Why remove this precondition? Line 75: // Map of ports to process environments. Never removed from. Used to restart processes. Sorry to pick on you for this (if it helps, I've bugged JD too), but this proliferation of map<pid, ...> is kind of nuts. Can we model a process and all this extra metadata with a class and use a single map? The new class could have all public fields for all I care; I just want it encapsulated in one place instead of strewn out across a bunch of maps. Line 220: String spn = "kudu/" + bindHost; Nit: can you inline this two lines below? Line 299: String spn = "kudu/" + bindHost; Nit: inline Line 320: * Starts a process using the provided command and configures it to be daemon, Update. PS1, Line 336: LOG.info("Starting process: {}, environment: {}", : Joiner.on(" ").join(processBuilder.command()), : Joiner.on(",").withKeyValueSeparator("=").join(processBuilder.environment())); This is also in MiniKdc.java. Perhaps it can be decomposed into a helper method? Line 495: LOG.warn(String.format("Could not delete path %s", path), e); Did the old call not work properly? Line 547: public Subject getLoggedInSubject() { Nit: Javadoc. http://gerrit.cloudera.org:8080/#/c/5150/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKdc.java: Line 43: assertFalse(klist.contains("[email protected]")); Why did this change? -- To view, visit http://gerrit.cloudera.org:8080/5150 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5131edb1f2bda443f7980a4aad86362666b3b6f5 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
