Mike Percy has posted comments on this change. Change subject: EMC: Don't reuse data dir for log dir ......................................................................
Patch Set 3: (13 comments) http://gerrit.cloudera.org:8080/#/c/5619/3//COMMIT_MSG Commit Message: PS3, Line 11: . > It would be nice if we could elaborate the problem with current directory s added more details 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 i added more details 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 Done PS3, Line 809: process > Do we want to note the addition of this in the COMMIT_MSG ? hmm, actually this belongs in the following patch. removed. http://gerrit.cloudera.org:8080/#/c/5619/1/src/kudu/integration-tests/external_mini_cluster.h File src/kudu/integration-tests/external_mini_cluster.h: Line 338: explicit ExternalDaemon(ExternalDaemonOptions opts); > warning: single-argument constructors must be marked explicit to avoid unin Done PS1, Line 404: bool > no need to return a const primitive Done Line 499: explicit ExternalMaster(ExternalDaemonOptions opts); > warning: single-argument constructors must be marked explicit to avoid unin Done 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 Done Line 327: struct ExternalDaemonOptions { > Outside of EMC, no one is supposed to interact with this, right? Would be g That would have the effect of making ExternalDaemon, as well as ExternalMaster and ExternalTabletServer unconstructible by anyone other than EMC. Since these are just test classes, and we mostly trust ourselves, I don't see the danger in leaving it public. Line 332: bool log_to_stderr = false; > Can we omit the default value? This should always be set to the value of E Done Line 352: Subprocess* process() const; > What do you need this for? Not seeing it used in the patch. you're right, moved out to the following 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? Nobody is using it yet, so I'll just remove 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? C Done -- 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 <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
