Adar Dembo has posted comments on this change. Change subject: log_block_manager: fix corruption after re-opening compacted metadata ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/7113/1/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 1208: TEST_F(LogBlockManagerTest, TestDeleteFromContainerAfterMetadataCompaction) { > warning: redundant 'test_info_' declaration [readability-redundant-declarat Maybe add the word "Fuzz" in there, since it's sort of a fuzz test (i.e. the random deletions are meant to generate a different set of cache hits/misses every run). http://gerrit.cloudera.org:8080/#/c/7113/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 2330: file_cache_.Evict(metadata_file_name); Curious as to whether you thought about implementing the rename through the file cache? For this single-threaded non-concurrent case an explicit Evict() is fine, but if done through the cache it seems like it might be easier to support a concurrent use case (i.e. for realtime metadata compaction). http://gerrit.cloudera.org:8080/#/c/7113/1/src/kudu/util/file_cache-test.cc File src/kudu/util/file_cache-test.cc: Line 227: // Create a test file, then delete it. It will be deleted immediately. Comment doesn't really make sense; we're renaming the file, not deleting it. http://gerrit.cloudera.org:8080/#/c/7113/1/src/kudu/util/file_cache.h File src/kudu/util/file_cache.h: PS1, Line 147: It's recommended that the above pattern : // only be used in cases when it can be guaranteed that no other objects : // retain an open descriptor to the same path. Can we enforce that there are no open readers of this file at the time of eviction? Would the LBM be okay with such semantics? -- To view, visit http://gerrit.cloudera.org:8080/7113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I14b2c64685e24d27591258911db4aeb9e8020a4d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
