Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8162 )
Change subject: KUDU-2055 [part 4]: Coalesce hole punch for LBM ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.h@299 PS2, Line 299: The deletion is withdrawn for blocks belong to read-only container. 'Blocks belonging to read-only containers will not be deleted. http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1167 PS2, Line 1167: hole punching s/hole punching/one hole punch http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1174 PS2, Line 1174: void UpdateDeletedBlockMap(const scoped_refptr<internal::LogBlock>& lb); I think this would be more clear if it were named 'AddBlock'. 'Update' is ambiguous, it could be an add or remove. http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1196 PS2, Line 1196: at s/at/in http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1202 PS2, Line 1202: * with the type http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1207 PS2, Line 1207: container->block_manager()->metrics()->holes_punched->Increment(); Hoist this check/increment out of the loop and use IncrementBy http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1220 PS2, Line 1220: LogBlockManager* lbm = down_cast<LogBlockManager*>(bm_); Since there are separate implementations of the transaction types now, perhaps it makes sense to store bm_ as the more specific type instead of down casting? http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@1283 PS2, Line 1283: transaction_ = transaction->ptr(); Can this be simplified to transaction_ = transaction; At which point you don't need ptr()? http://gerrit.cloudera.org:8080/#/c/8162/2/src/kudu/fs/log_block_manager.cc@2027 PS2, Line 2027: // Deletion is forbidden for read-only container. This comment should be moved down to the read_only() check -- To view, visit http://gerrit.cloudera.org:8080/8162 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ccc9538b8184f8f7ba8f6118713b82fef433275 Gerrit-Change-Number: 8162 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Fri, 29 Sep 2017 18:52:21 +0000 Gerrit-HasComments: Yes
