Hao Hao has posted comments on this change. Change subject: log block manager: mark container as read-only after syncing error ......................................................................
Patch Set 13: (20 comments) http://gerrit.cloudera.org:8080/#/c/7374/12//COMMIT_MSG Commit Message: PS12, Line 7: log block manager: mark container as read-only after syncing : error > Since this is almost ready to go, remove the WIP prefix. Done PS12, Line 15: For instance, suppose that SyncData() fails during Close() of : block A. Even though Close() itself will fail, it's possible : that block A's metadata was successfully written out to disk. : Some Kudu operations (e.g. merge compaction) will not crash : during a block error like this one, and if block A's container > Rewrite: Done PS12, Line 21: > Nit: fix 'read_only' here too. Done PS12, Line 22: > metadata Done http://gerrit.cloudera.org:8080/#/c/7374/12/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 843: // Regression test for KUDU-1793. This test simulates the case > But we're no longer testing failures in Append(); that seems like important Right, but if I put fault injection starting from append, the test will pass as I guess most of the cases it failed before reaching Close(). And as the test is mainly for testing on disk metadata points to wrong data because of incorrect in-memory accounting, so I left out Append() and only do fault injection around FlushDataAsync() and Close(). Maybe you have better suggestions? PS12, Line 844: metdata > metadata Done Line 864: // Creates a block with the given 'test_data', writing the result > Nit: update to reflect the effect of 'test_data'. Done Line 875: // and Close(). > Hmm, I wonder if using a FlagSaver on every block creation is expensive. Co So I measured the runtime, before the change it is around 4 seconds. and after the change it is around 22 seconds. But I am not sure if the difference is because of FlagSaver. Or it is because we only do fault injection around FlushDataAsyncClose() which will increase the runtime. Line 884: > We've also lost test coverage here by not actually verifying the result of I cannot think of a good solution for this, do you have some suggestions? PS12, Line 909: Status s = create_a_block(&id, i % 2 ? kShortTestData : kLongTestData); : : if (s.ok()) { : InsertOrDie(&ids, id); : > Nit: use a ternary: Done http://gerrit.cloudera.org:8080/#/c/7374/8/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1206: state_ = DIRTY; > oh I see, that's very subtle. Could you add a comment to that effect above Sure, will do. http://gerrit.cloudera.org:8080/#/c/7374/12/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS12, Line 378: read-only > Nit: I see read only and 'read_only' in the various comments you've added t Done PS12, Line 474: That means blocks : // cannot be deleted until next restart. > And the container won't receive new blocks, right? Done PS12, Line 771: te > Nit: don't need 'and'. Done PS12, Line 780: > marks Done PS12, Line 788: // : // On the other hand, the new blocks' metadata may be > The double negative in this sentence makes it tougher to read, please rewri Done PS12, Line 1156: > read-only or read only Done PS12, Line 1645: kManager::DeleteB > "is forbidden" Done Line 1656: } > See my other comment on how to phrase this. Done Line 1660: lb->container()->ToString()); > Why acquire lock_ and do the map lookup twice? Please rewrite this so there Done -- 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: 13 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
