Adar Dembo has posted comments on this change. Change subject: EMC: Don't reuse data dir for log dir ......................................................................
Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/5619/3//COMMIT_MSG Commit Message: PS3, Line 13: This change is needed in order to integrate Breakpad in a later change, : since creating the necessary directory structure for Breakpad minidump : files in the shared log/data directory at startup time confuses : FsManager when we ask it to then initialize the data directory. This is a bit confusing; are you saying that 1) initializing the breakpad infrastructure creates files in the log directory (because that's where the breakpad files end up) and 2) the breakpad infra must be initialized before the FsManager (note: it's also not instinctively clear why this must be the case)? If so, could you spell that out more explicitly? If not, could you elaborate on how files end up in the (previously) shared log/data directory without a crash? http://gerrit.cloudera.org:8080/#/c/5619/3/src/kudu/integration-tests/external_mini_cluster.cc File src/kudu/integration-tests/external_mini_cluster.cc: Line 652: if (!env->FileExists(log_dir_)) { CreateDirsRecursively() will no-op if it doesn't exist; let's just drop the check. http://gerrit.cloudera.org:8080/#/c/5619/3/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: Line 27: #include <boost/optional.hpp> I think this can be removed now; could you check other places in EMC where it may be in use and remove them too? Line 327: struct ExternalDaemonOptions { Outside of EMC, no one is supposed to interact with this, right? Would be good to enforce that. We could make it a protected member of ExternalDaemon, and make ExternalMiniCluster a friend of ExternalDaemon. Line 332: bool log_to_stderr = false; Can we omit the default value? This should always be set to the value of ExternalMiniClusterOptions.logtostderr. We could enforce that by adding a single-arg constructor whose arg is the ExternalMiniClusterOptions itself; then there'd be no way to create an ExternalDaemonOptions with an unset log_to_stderr. Line 352: Subprocess* process() const; What do you need this for? Not seeing it used in the patch. PS3, Line 403: // Returns true if the daemon will log to stderr. : bool logtostderr() const { return logtostderr_; } Also not seeing this used; why do we need it? http://gerrit.cloudera.org:8080/#/c/5619/3/src/kudu/integration-tests/master_migration-itest.cc File src/kudu/integration-tests/master_migration-itest.cc: PS3, Line 113: if (!env_->FileExists(DirName(data_root))) { : ASSERT_OK(env_->CreateDir(DirName(data_root))); : } It's a guarantee that master-1/data and master-2/data don't exist, right? Can we remove the FileExists() check? -- To view, visit http://gerrit.cloudera.org:8080/5619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba23c429989df524da51eb012a491766df06e955 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
