Adar Dembo has posted comments on this change. Change subject: log block manager: mark container as read-only after syncing error ......................................................................
Patch Set 13: (9 comments) 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 > Right, but if I put fault injection starting from append, the test will pas The reason I care about this is that the implementation used to update in-memory state in Append() (when KUDU-1793 was fixed). The test is somewhat "black box" in that it doesn't assume anything about which methods make in-memory updates; it basically assumes that any of them can. After your changes, with a 10% failure rate and kNumAppends=1000 it's no surprise that an Append() always fails. Can we rejigger the failure rate and/or kNumAppends to make it possible but not guaranteed that an Append() fails? Line 875: // and Close(). > So I measured the runtime, before the change it is around 4 seconds. and af 22 seconds means this should be converted into a slow-mode test. But, perhaps it's because of the number of appends/tries? Maybe that could be adjusted? Line 884: > I cannot think of a good solution for this, do you have some suggestions? The data read back is some sequence of kShortTestData and kLongTestData substrings, right? Maybe you could iterate on it and verify that? http://gerrit.cloudera.org:8080/#/c/7374/13/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS13, Line 474: That means blocks : // cannot be deleted until next restart. And new blocks cannot be : // added to the container neither. Please reword: "Existing blocks may not be deleted until the next restart, and new blocks may not be added." PS13, Line 770: 'read_only' Fix. PS13, Line 790: despite Either "in spite of" or "despite", but not "despite of". Line 1157: return Status::IllegalState("container $0 is read-only", container_->ToString()); Actually I think Aborted here makes sense (since we're in Abort()); it's just in DeleteBlock() that I think it doesn't make sense. PS13, Line 1213: 'read_only' Fix. Line 1855: blocks_by_block_id_.erase(lb->block_id()); This isn't nearly as efficient as passing in a const_iterator. Could you refactor the deletion so that you get an iterator and use it for deletion here? -- 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
