Hao Hao has posted comments on this change.

Change subject: log block manager: mark container as read-only after syncing 
error
......................................................................


Patch Set 17:

(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.
> The reason I care about this is that the implementation used to update in-m
Right, I have adjusted the kNumAppends times and was able to reproduce the 
failure with the test. Thanks!


Line 875:     *out = block->id();
> 22 seconds means this should be converted into a slow-mode test. But, perha
Not with the adjustment, it took around 7 seconds. Does this sound good?


Line 884:     RETURN_NOT_OK(this->bm_->OpenBlock(id, &block));
> The data read back is some sequence of kShortTestData and kLongTestData sub
Thanks, will do 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: Existing blocks may
              :   // not be deleted until the next restart, and new blocks may 
not
              :   // be added.
> Please reword:
Done


PS13, Line 770: read-only t
> Fix.
Sorry that I just realized I missed this when sent out patch set 13. So I fixed 
this and all the other ones in the latest patch set 14.


PS13, Line 790: despite
> Either "in spite of" or "despite", but not "despite of".
Done


Line 1157:     return Status::Aborted("container $0 is read-only", 
container_->ToString());
> Actually I think Aborted here makes sense (since we're in Abort()); it's ju
Done


PS13, Line 1213: read-only t
> Fix.
Fixed in the latest patch.


Line 1855:   scoped_refptr<LogBlock> lb = std::move(it->second);
> This isn't nearly as efficient as passing in a const_iterator. Could you re
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: 17
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

Reply via email to