Adar Dembo has posted comments on this change. Change subject: log block manager: reopen container metadata writers at the OS level ......................................................................
Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: PS1, Line 292: OpenContainerMetadataWriter > docs Done http://gerrit.cloudera.org:8080/#/c/6825/1/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: PS1, Line 649: DCHECK_EQ(FileState::NOT_INITIALIZED, state_); : RETURN_NOT_OK(ParsePBFileHeader(writer_.get(), &offset_, &version_)); : ContainerSupHeaderPB sup_header; : RETURN_NOT_OK(ReadSupplementalHeader(writer_.get(), version_, &offset_, &sup_header)); : RETURN_NOT_OK(writer_->Size(&offset_)); // Reset the write offset to the end of the file. : state_ = FileState::OPEN; > why was this thread safe before and what changed that it doesn't need to be (I'm sure you're already aware of most of this long explanation, but I'm leaving it intact for the benefit of others) 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, Open() is just the equivalent of Init(), and so it makes sense for it to be just as thread unsafe as Init() 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.). -- To view, visit http://gerrit.cloudera.org:8080/6825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6029d499422f5a2db036d796c7e208f2af71c394 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
