Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 17: (10 comments) I think I'm done reviewing everything outside log_block_manager.cc. Still a lot to take in there. I've been making my way through it, but thought I'd publish my comments so far. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: Line 892: TEST_P(TestCFileBothCacheTypes, TestReleaseBlock) { What happened to parameterizing this test for all block_time_to_flush values? In my earlier comment I suggested that doing so would be moot only if there was no block_time_to_flush-specific code in here. But there is such code, so we should do the parameterization so as to avoid bit-rot. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: PS17, Line 266: if (FLAGS_block_time_to_flush == "finalize") { : RETURN_NOT_OK(block_->Finalize()); : } else if (FLAGS_block_time_to_flush != "close" && : FLAGS_block_time_to_flush != "none") { : LOG(FATAL) << "Unknown value for block_time_to_flush: " : << FLAGS_block_time_to_flush; : } This is incorrect. We should always Finalize() the block; whether or not we flush inside Finalize() is determined by the block manager (and the value of this gflag). http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/block_manager.cc File src/kudu/fs/block_manager.cc: Line 43: DEFINE_string(block_time_to_flush, "finalize", > warning: redundant 'FLAGS_block_time_to_flush' declaration [readability-red Let's rename to "block_manager_flush_control": 1. "block_manager_" is the prefix we've been using for other BM-related flags. 2. "time_to_flush" sounds conspicuously like time to live and other such numerical values. Line 44: "When to flush blocks registered in a BlockTransaction. " This should apply to any WritableBlock, not just those in a BlockTransaction. PS17, Line 48: none Let's use "never" instead, since we're talking about a point in time. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 32: DECLARE_string(block_time_to_flush); This doesn't belong here (block_coalesce_close didn't belong either). http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: Line 340: RETURN_NOT_OK_HANDLE_ERROR(writer_->Flush(WritableFile::FLUSH_ASYNC)); This is where we should check the value of block_time_to_flush. We should only flush if it's 'finalize'. http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1176: LOG(WARNING) << Substitute( I mentioned this to Dan in our impromptu meeting yesterday; perhaps you weren't yet there: we should remove this WARNING because with out-of-order block metadata on disk it'll fire more frequently. PS17, Line 1397: container_->block_manager() You can use 'lbm' here. Line 1410: RETURN_NOT_OK(container_->FlushData(block_offset_, block_length_)); And here we also need to gate on block_time_to_flush == 'finalize'. -- 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: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
