You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/30 19:20:19 UTC
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Hello Dan Burkert, Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5276
to review the following change.
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
KUDU-1551. Ignore log segments which were preallocated but have no header.
This fixes a TS startup crash in the case that it finds a log segment
which is preallocated but has no valid magic or header at the start.
The new test injects such a crash and verifies that the server can
restart and replay its logs.
Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/ts_recovery-itest.cc
5 files changed, 63 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/5276/1
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
KUDU-1551. Ignore log segments which were preallocated but have no header.
This fixes a TS startup crash in the case that it finds a log segment
which is preallocated but has no valid magic or header at the start.
The new test injects such a crash and verifies that the server can
restart and replay its logs.
Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Reviewed-on: http://gerrit.cloudera.org:8080/5276
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/ts_recovery-itest.cc
5 files changed, 66 insertions(+), 14 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5276
to look at the new patch set (#2).
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
KUDU-1551. Ignore log segments which were preallocated but have no header.
This fixes a TS startup crash in the case that it finds a log segment
which is preallocated but has no valid magic or header at the start.
The new test injects such a crash and verifies that the server can
restart and replay its logs.
Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/ts_recovery-itest.cc
5 files changed, 65 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/5276/2
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5276
to look at the new patch set (#3).
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
KUDU-1551. Ignore log segments which were preallocated but have no header.
This fixes a TS startup crash in the case that it finds a log segment
which is preallocated but has no valid magic or header at the start.
The new test injects such a crash and verifies that the server can
restart and replay its logs.
Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
M src/kudu/integration-tests/ts_recovery-itest.cc
5 files changed, 66 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/5276/3
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5276/2/src/kudu/consensus/log_reader.cc
File src/kudu/consensus/log_reader.cc:
Line 146: if (s.IsUninitialized()) {
> Do these segments get deleted?
we re-write all the log segments during playback, and then delete the old directory, so yes, but not in an explicit way.
http://gerrit.cloudera.org:8080/#/c/5276/2/src/kudu/integration-tests/ts_recovery-itest.cc
File src/kudu/integration-tests/ts_recovery-itest.cc:
Line 206: // Enable the fault point after creating the table, but before writing any data.
> Why is it important to do enable the fault after table creation? If it's do
yep.
PS2, Line 212: MonoDelta::FromSeconds(30)
> Is this going to be enough time on ASAN/TSAN?
I think so, since I set the roll size to 1MB. It's sub-second on a debug build, so I'd guess that even on ASAN/TSAN it would only be a few seconds. I'll double check.
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1551. Ignore log segments which were preallocated but have no header.
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1551. Ignore log segments which were preallocated but have no header.
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5276/2/src/kudu/consensus/log_reader.cc
File src/kudu/consensus/log_reader.cc:
Line 146: if (s.IsUninitialized()) {
Do these segments get deleted?
PS2, Line 149: .
Nit: extra period here?
http://gerrit.cloudera.org:8080/#/c/5276/2/src/kudu/consensus/log_util.cc
File src/kudu/consensus/log_util.cc:
PS2, Line 473: ""
Can we put something useful here?
http://gerrit.cloudera.org:8080/#/c/5276/2/src/kudu/integration-tests/ts_recovery-itest.cc
File src/kudu/integration-tests/ts_recovery-itest.cc:
Line 206: // Enable the fault point after creating the table, but before writing any data.
Why is it important to do enable the fault after table creation? If it's done at the very beginning, the tserver may crash in work.Setup()?
PS2, Line 212: MonoDelta::FromSeconds(30)
Is this going to be enough time on ASAN/TSAN?
--
To view, visit http://gerrit.cloudera.org:8080/5276
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I843cf483b93823cbcc5506958f62cbb076569ca8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes