Dan Burkert has posted comments on this change. Change subject: [WIP]log block manager:mark container as 'read_only' after syncing error ......................................................................
Patch Set 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/7374/8//COMMIT_MSG Commit Message: PS8, Line 7: : add a space after the colon PS8, Line 11: get s/get/is http://gerrit.cloudera.org:8080/#/c/7374/8/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 848: const string kLongTestData = "longtestdata"; Would this test be more effective if the difference between short and long were bigger? You can easily do this by using something like const string kLongTestData = string(300, 'L'); PS8, Line 851: small I think it was more correct before as 'so little' PS8, Line 870: around s/around/in http://gerrit.cloudera.org:8080/#/c/7374/8/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 266: void FinishBlock(Status* s, WritableBlock* block); Does the signature have to change here? I think it's a little more ambiguous what's happening with two out-params. Line 379: void SetMode(bool read_only); Perhaps it should be called 'SetReadOnly()' so there isn't any ambiguity about what a mode is. Line 772: if (!(*s).ok()) { s->ok() Line 773: read_only_.Store(true); Use SetMode/SetReadOnly? Line 779: if (!(*s).ok()) { This can use RETURN_NOT_OK Line 794: *s = block_manager()->SyncContainer(*this); Do we want to continue through to below in the case of failure? Line 833: DCHECK(!read_only_.Load()); Here and below, consider using read_only() Line 1206: auto cleanup = MakeScopedCleanup([&]() { Instead of creating a scoped cleanup object, and thus having to always remember to set 's', consider wrapping the logic of this method in a lambda, which is immediately executed, and returns a Status. That way you can be sure you never forget to set s: Status LogWritableBlock::FlushDataAsync() { Status s = [this] -> Status { <put all logic here> }(); if !s.ok() { MarkReadOnly(); } return s; Status s; } -- To view, visit http://gerrit.cloudera.org:8080/7374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I15062b9d82c9cb563fb6bb2af7ec89da5f71e28f Gerrit-PatchSet: 8 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
