Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8465 )

Change subject: KUDU-2202 avoid block ID reuse for missing dirs
......................................................................


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8465/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8465/1//COMMIT_MSG@10
PS1, Line 10: If the opening of a directory fails (e.g.
            : due to a disk failure), the server can still start up, but will 
fail any
            : tablet configured to use that directory. By not scanning the 
failed
            : directory, the server will "forget" about some blocks.
            :
> I'd say that the server will still start up but fail any tablet configured
Done


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/fs/block_manager.h@264
PS1, Line 264:  to
> should
Done


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/fs/block_manager.h@266
PS1, Line 266:   virtual void NotifyBlockId(BlockId block_id) = 0;
> Since BlockIds are basically uint64_ts, we should just pass them by copy.
Done


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/integration-tests/ts_recovery-itest.cc
File src/kudu/integration-tests/ts_recovery-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/integration-tests/ts_recovery-itest.cc@144
PS1, Line 144:   if (GetParam() != "log") {
             :     LOG(INFO) << "Missin
> Where's the multi-directory part?
Was originally structured to fail a directory. No longer!


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/integration-tests/ts_recovery-itest.cc@199
PS1, Line 199:   for (const string& dir : ts->data_dirs()) {
             :     const string& data_dir = JoinPathSegments(dir, "data");
             :     vector<string> children;
> This will fail the old tablet, right? Is that worth asserting for?
Done


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/integration-tests/ts_recovery-itest.cc@207
PS1, Line 207:
> You can get this special string from data_dirs.h (or should be able to, any
Done


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/integration-tests/ts_recovery-itest.cc@208
PS1, Line 208:
> ASSERT_OK?
Done


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/integration-tests/ts_recovery-itest.cc@233
PS1, Line 233:                         new_block_ids.begin(), 
new_block_ids.end(),
> You could SCOPED_TRACE this (and the first/second sets) to avoid logging th
Done


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/tablet/tablet_metadata.cc@399
PS1, Line 399:       next_rowset_idx_ = std::max(next_rowset_idx_, 
rowset_meta->id() + 1);
> I'm curious whether this plumbing the max "down" like this is more efficien
Ran a few runs of dense_node-itest and it seems like it doesn't make a huge 
difference.
Good call though, bootstrap is dominated by disk access, the difference in the 
O(N) copy here is pretty negligible. WiIl go with that approach.


http://gerrit.cloudera.org:8080/#/c/8465/1/src/kudu/tablet/tablet_metadata.cc@413
PS1, Line 413: ager of blocks it may not be aware of (e.g. if a
             :     // disk was not read).
> Not really necessary for this comment.
Done



--
To view, visit http://gerrit.cloudera.org:8080/8465
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06f10e95278dafdedcab432a7d4d1dc5c59bf4cc
Gerrit-Change-Number: 8465
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 04 Nov 2017 01:13:07 +0000
Gerrit-HasComments: Yes

Reply via email to