Mike Percy has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 22: (13 comments) Looks pretty good to me, I mainly have nits. http://gerrit.cloudera.org:8080/#/c/7207/22//COMMIT_MSG Commit Message: PS22, Line 15: for an http://gerrit.cloudera.org:8080/#/c/7207/22/src/kudu/fs/block_manager.cc File src/kudu/fs/block_manager.cc: PS22, Line 34: blocks block Line 45: DEFINE_string(block_manager_flush_control, "finalize", I don't like the term flush because it doesn't distinguish between a memory buffer flush vs. an fsync. Also, from the description here it sounds like 'never' means the blocks will never be fsynced. We should clarify that this is an implicit pre-flush or pre-sync, that the global settings for syncing will still be used at close time. Perhaps we should call this flag block_manager_preflush_control instead, or block_manager_early_sync_control. PS22, Line 46: when to flush More like when to pre-flush? http://gerrit.cloudera.org:8080/#/c/7207/22/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS22, Line 70: is is set to PS22, Line 127: is is set to PS22, Line 128: asynchronous flush of dirty block data to disk Sounds good for performance. What is the crash consistency contract? Can this leak disk space? PS22, Line 283: BlockTransaction() = default; : : ~BlockTransaction() = default; nit: It seems superfluous to specify = default here, particularly since these are not something special like copy or move constructors. Am I missing something? http://gerrit.cloudera.org:8080/#/c/7207/22/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 276: // This is called after synchronization of dirty data and metadata doesn't finalization have to happen before the data is synced to disk? Line 355: // If successful, adds all blocks to the block manager's in-memory maps. and takes ownership of the LogWritableBlock instances, right? PS22, Line 458: rounds up "rounds up"? PS22, Line 459: it clarify: is "it" the block or the container? PS22, Line 1119: due to KUDU-1793 That issue is marked as RESOLVED. Is this still an issue? -- 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: 22 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: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
