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/12/04 08:22:55 UTC
[kudu-CR] KUDU-1782. Fault injection crashes should exit with a specific exit code
Hello David Ribeiro Alves,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5353
to review the following change.
Change subject: KUDU-1782. Fault injection crashes should exit with a specific exit code
......................................................................
KUDU-1782. Fault injection crashes should exit with a specific exit code
This changes the fault injection "crash" to use exit(85) instead of
LOG(FATAL). This means that we can now distinguish between a process
exiting due to fault injection compared to a process exiting due to a
crash or real FATAL-inducing issue.
The WaitForCrash method in ExternalMiniCluster::ExternalDaemon is now
renamed to WaitForInjectedCrash and specifically checks that the exit
code matches the expected one. If not, it now returns a bad status.
I verified the new functionality by locally reverting back the fault
injection code to use LOG(FATAL) and checking that ts_recovery-itest
reported that the process had crashed unexpectedly.
Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/util/fault_injection.cc
M src/kudu/util/fault_injection.h
8 files changed, 45 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/5353/1
--
To view, visit http://gerrit.cloudera.org:8080/5353
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] KUDU-1782. Fault injection crashes should exit with a specific exit code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1782. Fault injection crashes should exit with a specific exit code
......................................................................
KUDU-1782. Fault injection crashes should exit with a specific exit code
This changes the fault injection "crash" to use exit(85) instead of
LOG(FATAL). This means that we can now distinguish between a process
exiting due to fault injection compared to a process exiting due to a
crash or real FATAL-inducing issue.
The WaitForCrash method in ExternalMiniCluster::ExternalDaemon is now
split into WaitForInjectedCrash, which specifically checks that the exit
code matches the expected one, and WaitForFatal, which checks that the
exit code matches a SIGABRT. If the process exits with an unexpected
status, a bad Status is returned, typically resulting in a test failure.
I verified the new functionality by locally reverting back the fault
injection code to use LOG(FATAL) and checking that ts_recovery-itest
reported that the process had crashed unexpectedly.
Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
Reviewed-on: http://gerrit.cloudera.org:8080/5353
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/util/fault_injection.cc
M src/kudu/util/fault_injection.h
8 files changed, 90 insertions(+), 31 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5353
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1782. Fault injection crashes should exit with a specific exit code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1782. Fault injection crashes should exit with a specific exit code
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5353/3/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:
Line 494: ASSERT_OK(cluster_->tablet_server(kTsIndex)->Restart());
> Seems like a pretty important omission. What was the effect?
basically made this part of the test a no-op: the WaitForTSToCrash() below used to silently ignore if the TS wasn't started at that point, whereas I added a CHECK in this patch that verifies it's in the expected state.
--
To view, visit http://gerrit.cloudera.org:8080/5353
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1782. Fault injection crashes should exit with a specific exit code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5353
to look at the new patch set (#2).
Change subject: KUDU-1782. Fault injection crashes should exit with a specific exit code
......................................................................
KUDU-1782. Fault injection crashes should exit with a specific exit code
This changes the fault injection "crash" to use exit(85) instead of
LOG(FATAL). This means that we can now distinguish between a process
exiting due to fault injection compared to a process exiting due to a
crash or real FATAL-inducing issue.
The WaitForCrash method in ExternalMiniCluster::ExternalDaemon is now
split into WaitForInjectedCrash, which specifically checks that the exit
code matches the expected one, and WaitForFatal, which checks that the
exit code matches a SIGABRT. If the process exits with an unexpected
status, a bad Status is returned, typically resulting in a test failure.
I verified the new functionality by locally reverting back the fault
injection code to use LOG(FATAL) and checking that ts_recovery-itest
reported that the process had crashed unexpectedly.
Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/util/fault_injection.cc
M src/kudu/util/fault_injection.h
8 files changed, 84 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/5353/2
--
To view, visit http://gerrit.cloudera.org:8080/5353
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1782. Fault injection crashes should exit with a specific exit code
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1782. Fault injection crashes should exit with a specific exit code
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5353/3/src/kudu/integration-tests/delete_table-test.cc
File src/kudu/integration-tests/delete_table-test.cc:
Line 494: ASSERT_OK(cluster_->tablet_server(kTsIndex)->Restart());
Seems like a pretty important omission. What was the effect?
--
To view, visit http://gerrit.cloudera.org:8080/5353
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1782. Fault injection crashes should exit with a specific exit code
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5353
to look at the new patch set (#3).
Change subject: KUDU-1782. Fault injection crashes should exit with a specific exit code
......................................................................
KUDU-1782. Fault injection crashes should exit with a specific exit code
This changes the fault injection "crash" to use exit(85) instead of
LOG(FATAL). This means that we can now distinguish between a process
exiting due to fault injection compared to a process exiting due to a
crash or real FATAL-inducing issue.
The WaitForCrash method in ExternalMiniCluster::ExternalDaemon is now
split into WaitForInjectedCrash, which specifically checks that the exit
code matches the expected one, and WaitForFatal, which checks that the
exit code matches a SIGABRT. If the process exits with an unexpected
status, a bad Status is returned, typically resulting in a test failure.
I verified the new functionality by locally reverting back the fault
injection code to use LOG(FATAL) and checking that ts_recovery-itest
reported that the process had crashed unexpectedly.
Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
---
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/external_mini_cluster.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/util/fault_injection.cc
M src/kudu/util/fault_injection.h
8 files changed, 90 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/5353/3
--
To view, visit http://gerrit.cloudera.org:8080/5353
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8d9ae38efa65123ae69097a76c113b9709c3484
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot