You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/12/06 18:47:09 UTC
[kudu-CR] log block manage: tighten up read only usage
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5383
to review the following change.
Change subject: log block manage: tighten up read only usage
......................................................................
log block manage: tighten up read only usage
Over time we had introduced a couple new file deletion/truncation sites that
weren't checking read_only_ before making destructive changes.
Found via code inspection.
Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
---
M src/kudu/fs/log_block_manager.cc
1 file changed, 11 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/5383/1
--
To view, visit http://gerrit.cloudera.org:8080/5383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] log block manage: tighten up read only usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5383
to look at the new patch set (#2).
Change subject: log block manage: tighten up read only usage
......................................................................
log block manage: tighten up read only usage
Over time we had introduced a couple new file deletion/truncation sites that
weren't checking read_only_ before making destructive changes.
Found via code inspection.
Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 41 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/5383/2
--
To view, visit http://gerrit.cloudera.org:8080/5383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] log block manage: tighten up read only usage
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: log block manage: tighten up read only usage
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5383/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 410: if (!block_manager->read_only_ &&
Shouldn't this still return Status::Aborted if we are in read_only_ mode?
Line 499: if (read_status.IsIncomplete() && !block_manager()->read_only_) {
Same here - should we return OK but not attempt to repair it?
--
To view, visit http://gerrit.cloudera.org:8080/5383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1791: tighten up read only usage in log block manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-1791: tighten up read only usage in log block manager
......................................................................
KUDU-1791: tighten up read only usage in log block manager
Over time we had introduced a couple new file deletion/truncation sites that
weren't checking read_only_ before making destructive changes.
This was originally caught by open-fs-readonly-itest. Though exceptionally
rare, it's difficult to conceive of a different test that would surface the
issue more regularly.
Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Reviewed-on: http://gerrit.cloudera.org:8080/5383
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 41 insertions(+), 26 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1791: tighten up read only usage in log block manager
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5383
to look at the new patch set (#3).
Change subject: KUDU-1791: tighten up read only usage in log block manager
......................................................................
KUDU-1791: tighten up read only usage in log block manager
Over time we had introduced a couple new file deletion/truncation sites that
weren't checking read_only_ before making destructive changes.
This was originally caught by open-fs-readonly-itest. Though exceptionally
rare, it's difficult to conceive of a different test that would surface the
issue more regularly.
Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
---
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
2 files changed, 41 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/5383/3
--
To view, visit http://gerrit.cloudera.org:8080/5383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1791: tighten up read only usage in log block manager
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1791: tighten up read only usage in log block manager
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] log block manage: tighten up read only usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: log block manage: tighten up read only usage
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5383/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
Line 410: if (!block_manager->read_only_ &&
> Shouldn't this still return Status::Aborted if we are in read_only_ mode?
Not necessarily; we'll fail in one of the open calls below and Open() will still fail until the repair.
But, if we return Aborted, we'll safely ignore this container and carry on. That's clearly the better experience, so I'll make the change.
Line 499: if (read_status.IsIncomplete() && !block_manager()->read_only_) {
> Same here - should we return OK but not attempt to repair it?
See above: we could just fail outright. But, as long as we don't try to write to metadata_file_ (which we shouldn't, since this is a read-only instance), we can't corrupt anything.
--
To view, visit http://gerrit.cloudera.org:8080/5383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I029527b78148d68a002a3c42af5a52f179089a95
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes