Dinesh Bhat has posted comments on this change. Change subject: block_manager: consolidate data directory management ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS4, Line 73: reconfigure " : "Kudu with --block_manager=file At this point, 'reconfigure' means starting the kudu with a new fs_root_path or reformatting the fs before restarting the kudu on that node right ? Basically upon holePunchError, we can not restart the kudu-tserver and switch to FBM without following the pre-requisite cleanup steps, right ? Just wanted to make sure this error message is unambiguous to the log reader. Line 250: LOG(WARNING) << "Proceeding without lock"; This may be a relocated code, but I am curious the instance->LockFile() may have failed for number of reasons, and we still proceed without verifying if the file underneath had some other failure than fcntl(lock) related failure. Or may be we expect the file to have been opened by this stage and only failure we expect here is a fcntl(lock) , isn't it ? http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 77: class DataDirManager { Can we put one or two liner description for this to make it clear how this cab be leveraged by FBM and LBM ? http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 170: class LogBlockContainer { A basic C++ question not related to this change - was it intentional to put this class inside .cc instead of log_block_manager.h ? When would we normally do that ? PS4, Line 1153: OPTIONAL Just a brainstorm idea here(not suggesting for this patch): I wonder if we could do away with just 2 modes here: one which locks another which doesn't lock. I couldn't trace these modes to advisory vs mandatory locks for the instance files underneath. I may have missed it though. Line 1179: return s; Do we need to worry about cleaning up successfully opened data_dirs here ? Or perhaps we bank on LBM destructor to do that cleanup ? http://gerrit.cloudera.org:8080/#/c/4793/4/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: Line 308: DataDirManager data_dirs_; Nit: can we slightly modify the variable name to reflect that it's a manager instance and not data_dir instance ? At first look, looking at variable name alone, I thought this is incarnation of DataDir. -- To view, visit http://gerrit.cloudera.org:8080/4793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifc9d8999117a6383e7487b6c5bf065f10247b1d7 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
