Dan Burkert has posted comments on this change. Change subject: [java-client]: support for Kerberized RPC ......................................................................
Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/5150/1//COMMIT_MSG Commit Message: PS1, Line 10: ha > redundant Done 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 authenticate to Kerberized Kudu clusters. > Nit: "to connect to Kerberized..." maybe? Done 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 ar They have already diverged somewhat - see the additional restrictions that are being added on cipher types here due to Java restrictions. Unless you feel strongly I'm going to skip this, I don't think it would simplify things overall. Line 333: } > If you've removed redirectOutput() because PIPE is the default value, you c Done 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... It wasn't adding any information. Line 111 > Why remove this precondition? I briefly had a test that set the number of tservers to 0 for debugging (less log spew). I kept the removal since I don't see the point in restricting this. I use 0 tserver tests pretty heavily when testing the RPC layer for the rust client, for instance. Line 75: private final List<String> pathsToDelete = new ArrayList<>(); > Sorry to pick on you for this (if it helps, I've bugged JD too), but this p I actually made this a lot worse than it needed to be, I had replaced 'environments' (map of maps) with just a single environment, but forgot to finish removing all the uses. Now it's just a single additional map shared by the whole cluster. Line 220: } > Nit: can you inline this two lines below? Done Line 299: * Starts a process using the provided command and configures it to be daemon, > Nit: inline Done Line 320: thread.setName(Iterables.getLast(Splitter.on(File.separatorChar).split(command.get(0))) + ":" + port); > Update. Done PS1, Line 336: * Starts a previously killed master process on the specified port. : * @param port which port the master was listening on for RPCs : * @throws Exception > This is also in MiniKdc.java. Perhaps it can be decomposed into a helper me I ended up removing this. 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? We no longer allow multiple logged in client principals due to moving from DIR to FILE typed ccache. A similar change was made on the C++ side in 123b9918c7005f65010c5d431f4e3cac459e7a31 -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[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
