Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 10: (5 comments) Hao, Dan, and I had a long discussion about this patch, and I wanted to reproduce our conclusions here for everyone else: 1. The cfile and block performance-related knobs are useful for testing, but are not as important as having a good API and a robust implementation. IIUC, the resplitting of FlushDataAsync and Finalize was due to retaining all the existing flags; let's not do this. Let's stick to the earlier approach (where Finalize implies a flush), and adjust the flags if need be. 2. What flags are worth preserving? We identified the need for just one, which dictates when a block's data should be flushed. It has three values: "finalize" (default value), "close" (defers the flush to when the entire transaction is committed), and "none" (doesn't flush at all, "flushing" will happen when the transaction commits and files are fsynced). 3. When should AppendMetadata be called? One option is to do it at Finalize time. Another is to defer it to Close. The problem with doing it during Finalize is that it exacerbates the "1. AppendData, 2. AppendMetadata, 3. SyncData, 4. SyncMetadata" race: if we crash between #2 and #3, we may end up with just the metadata on disk, and if we AppendMetadata during Finalize, we have many more blocks' worth of metadata that can land on disk without corresponding data. 4. But AppendMetadata is called during Close, we may end up with out-of-order metadata entries on disk, since transactions could commit out of order. Do we care? We don't think so; the LBM implementation should already be robust to this. An alternative is to use an in-memory buffer to retain metadata order, but this didn't seem worth the complexity. 5. What to do about zero length blocks? Should their metadata be written, or skipped? The answer is: it must be written, or attempts to Close such blocks should fail. If for whatever reason we skipped the metadata append, we could end up with block IDs whose blocks can't be found on startup. http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 423: TEST_P(TestCFileBothCacheTypes, TestWrite100MFileInts) { You should be able to use this to replace TestCFileBothCacheTypes. Here's what I think you'll need to do: 1. Rewrite TestCFileBothCacheTypes to be an empty subclass of TestCFileBothCacheCloseTypes. 2. The first call to INSTANTIATE_TEST_CASE_P should be for TestCFileBothCacheTypes and its values should be two CFileDescriptors that vary the CacheType but both use the CLOSE CloseType. 3. The second call to INSTANTIATE_TEST_CASE_P should be for TestCFileBothCacheCloseTypes and should pass all four CFileDescriptors. After our long in-person discussion about other changes to this patch, it's possible all of this becomes moot because there's no longer a cfile flag to test. Line 449: If the default value of this gflag changes to true, we'll never test the false case. You should use a switch statement and explicitly set it to either true or false. After our long in-person discussion about other changes to this patch, it's possible all of this becomes moot because there's no longer a cfile flag to test. http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: PS16, Line 59: // - users could always change this to "close", which slows down throughput : // but may improve write latency. This part isn't quite right anymore. Should the entire comment be rewritten in light of the split into two gflags? After our long in-person discussion about other changes to this patch, it's possible this becomes moot because there's no longer a cfile flag. Though you may still need to move this explanation (and rewrite it). PS16, Line 63: Nit: got an extra space here. http://gerrit.cloudera.org:8080/#/c/7207/10/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 1288: // Ensure the same container has not been marked as available twice. > I was trying to check in the available_containers_by_data_dir_ map, there i Ah, I forgot that available_containers_by_data_dir_::value_type is a vector. So indeed, if we marked the container as available twice, you'd see two entries in that vector. Makes sense. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a88884bd Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
