Adar Dembo 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_pat This is logged from CheckHolePunch(), called via Create(). In both cases, we take care to delete intermediate files in the event of an error. The idea is that if you see this, you really could change your Kudu configuration to use --block_manager=file, restart, and go on your merry way. You don't need to reformat anything. Line 250: LOG(WARNING) << "Proceeding without lock"; > This may be a relocated code, but I am curious the instance->LockFile() may Under the hood, Lock() calls open() and fcntl(). We expect open() to succeed because we just called open() via LoadFromDisk() a few lines earlier. So that really just leaves fcntl(). Separately, I think it's reasonable that if you asked for LockMode::OPTIONAL, you get what you paid for (i.e. continuing in spite of any error). If there's something truly wrong, we'll probably fail during an I/O later on. 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 Done, though I tried to stay away from a "this is how it can be used" explanation since that sort of thing tends to get stale. 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 Yes, this is because LogBlockContainer is private to the LogBlockManager, so there's no reason to expose it to users of the LogBlockManager. PS4, Line 1153: OPTIONAL > Just a brainstorm idea here(not suggesting for this patch): I wonder if we The OPTIONAL/MANDATORY duality here is orthogonal to UNIX advisory/mandatory locking. Under the hood, all of the locking is advisory (see env_posix.cc). Here, MANDATORY means "a lock failure is a fatal failure" and OPTIONAL means "proceed despite a lock failure". Line 1179: return s; > Do we need to worry about cleaning up successfully opened data_dirs here ? Correct, we'll let the destructor clean them up. We could explicitly close them ourselves, but it's not strictly necessary. 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 manage Done -- 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
