Adar Dembo has submitted this change and it was merged. Change subject: log block manager: reopen container metadata writers at the OS level ......................................................................
log block manager: reopen container metadata writers at the OS level Another patch I'm working on compacts LBM metadata files at startup by writing compacted metadata into a temporary file, then overwriting the existing metadata file with it. For this to work properly, "reopening" a metadata file should actually reopen the file on the filesystem. This patch does just that. There should be no impact on any existing code; the reopening done after truncating a partial record from a metadata file is just as happy if done at the logical level (before) as at the physical level (now). By popular demand, I've attached below a long explanation of Kudu's approach to object initialization and thread safety that came up during code review. Because we don't use exceptions within Kudu, the initialization of an object is typically done in two phases: 1. Constructor, for operations that cannot fail. 2. Initializer function, for operations that may fail. In most objects, the initializer function is named Init(), returns a Status type, and needn't be thread safe because it is called right after the constructor, before the object is "made public" to other threads. Some objects offer two initializer functions: one to initialize a brand new object and one to initialize an object with existing state. The idea is that callers always use the constructor and then call one of the two initializer functions, depending on their use case. WritablePBContainerFile is one such object; Init() is for a brand new container file, and Open() is for a container file that already exists on disk. Previously, Reopen() served double duty: it was both the initializer function for files with existing state, and it was an arbitrary function (like Append()) that could be called at any time and by any thread. That second responsibility is why it was thread safe. Now, OpenExisting() is just the equivalent of CreateNew(), and so it makes sense for it to be just as thread unsafe as CreateNew() is. To be clear, the LBM was the only "arbitrary" caller to Reopen(), and the LBM didn't _need_ it to be thread safe. But, I think it was net less confusing for Reopen() to be thread safe (and thus equivalent in semantics to Append()) than for it to be thread unsafe (and thus exceptional when compared to Append(), Sync(), etc.). Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Reviewed-on: http://gerrit.cloudera.org:8080/6825 Reviewed-by: Todd Lipcon <t...@apache.org> Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves <davidral...@gmail.com> --- M src/kudu/fs/log_block_manager-test-util.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/util/pb_util-test.cc M src/kudu/util/pb_util.cc M src/kudu/util/pb_util.h 6 files changed, 33 insertions(+), 40 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Todd Lipcon: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
