Adar Dembo has posted comments on this change. Change subject: MiniKdc for C++ ......................................................................
Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/4752/1/src/kudu/security/mini_kdc.cc File src/kudu/security/mini_kdc.cc: Line 44: MiniKdc::MiniKdc() > Done Not done? Line 54: options_.data_root = JoinPathSegments(GetTestDataDirectory(), "krb5kdc"); > Might also be interesting to push some of this functionality into Subproces Yes? No? Line 85: for (const auto& location : search) { > kdb5_util and krb5kdc require the files to exist. I see. Two things: 1) Let's make sure the new integration test fails if an intrepid developer accidentally combines the two CreateKrb5Conf() calls. 2) Please add a comment here explaining why it's being called twice. Line 230: > We can't assume the /sbin is on the path. TryRunLsof does essentially the You're right; I meant, perhaps we can just run lsof with PATH=/sbin:$PATH as TryRunLsof() does, instead of resorting to finding it with GetBinaryPath(). I'm suggesting that because many of the paths tested in GetBinaryPath() are for finding Kerberos binaries; lsof will definitely not be there. http://gerrit.cloudera.org:8080/#/c/4752/6/src/kudu/security/mini_kdc.cc File src/kudu/security/mini_kdc.cc: Line 106: static const vector<string> kCommonLocations = { This isn't in sync with FindKdc.cmake. PS6, Line 123: << " realm: " << options_.realm : << " port: " << options_.port : << " data_root: " << options_.data_root; Nit: may be cleaner to encapsulate this as ToString() in MiniKdcOptions; that way when we add a new option, we just update ToString() to add it to the logging. Line 277: options_.port = port; Hmm, this one we may want to store separately, so we can remember that the user originally asked for an ephemeral port. http://gerrit.cloudera.org:8080/#/c/4752/6/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: PS6, Line 116: // Writes enough bytes to stdout and stderr concurrently that if Call() were : // fully reading them one at a time, the test would deadlock. I understand that it was easy to piggy-back on this test, but it was explicitly testing a deadlock that I don't believe is possible with stdin. PS6, Line 118: StdIn Nit: Stdin http://gerrit.cloudera.org:8080/#/c/4752/6/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: Line 531: const string& stdin_in) { If we want to support arbitrarily large stdin values, I think we'll need to integrate the write of stdin into ReadFdsFully (that is, use a poll loop and periodically writes more bytes to stdin and read bytes out of stdout/stderr). Writing more than 64k can block the writer, and if the subprocess is blocked writing to stdout/stderr for the same reason, we'll deadlock the two. For now I'm OK with some docs explaining the stdin limitation, though I'm also OK with doing that integration now (shouldn't be too hard). http://gerrit.cloudera.org:8080/#/c/4752/6/src/kudu/util/subprocess.h File src/kudu/util/subprocess.h: PS6, Line 108: subprocesses Nit: subprocess' Line 112: const std::string& stdin_in = ""); Nit: would prefer if stdin came first. Why? 1) It hews to the order of the fd numbers for stdin/stdout/stderr in Unix (0, 1, and 2). 2) OUT parameters should be listed after IN parameters. -- To view, visit http://gerrit.cloudera.org:8080/4752 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I63fc53eeaa1e40b217030adc1ca0c132f43a076c Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
