You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2017/07/15 00:11:00 UTC
[kudu-CR] disk failure: local testing for disk failure
Andrew Wong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7441
Change subject: disk failure: local testing for disk failure
......................................................................
disk failure: local testing for disk failure
This patch adds tests that disk failures are handled as expected on a
single server. The server should not crash.
Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
---
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/ts_disk_failure-test.cc
2 files changed, 147 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7441/1
--
To view, visit http://gerrit.cloudera.org:8080/7441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] disk failure: local testing for disk failure
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7441
to look at the new patch set (#2).
Change subject: disk failure: local testing for disk failure
......................................................................
disk failure: local testing for disk failure
This patch adds tests that disk failures are handled as expected on a
single server. Various tablet operations (flushes, scans, compactions,
startup) are tested to ensure that an IOError is returned when
failure-injection is on. The server should not crash.
Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
---
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/ts_disk_failure-test.cc
2 files changed, 147 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7441/2
--
To view, visit http://gerrit.cloudera.org:8080/7441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] disk failure: local testing for disk failure
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: disk failure: local testing for disk failure
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7441/3/src/kudu/tserver/ts_disk_failure-test.cc
File src/kudu/tserver/ts_disk_failure-test.cc:
Needs copyright header.
Line 52: string GlobForTabletInDir(const DataDir* dir) {
I don't really understand the function name; what's the 'tablet' part of what it does? Seems like it's globbing data directory files or something like that.
Line 54: return JoinPathSegments(dir->dir(), Substitute("$0/**/$1", string(2, '?'), string(16, '?')));
Would ??/** suffice? Doesn't that glob all files within the globbed subdirectories too?
PS3, Line 71: ~TSDiskFailureTest() {
: FLAGS_env_inject_eio = 0;
: }
Not really necessary because every KuduTest instantiates a FlagSaver.
PS3, Line 83: InsertTestRowsDirect(1, 100);
: InsertTestRowsDirect(101, 100);
: InsertTestRowsDirect(201, 100);
Why three different batches of inserts? Won't one (with just one row) suffice?
Line 95: InsertTestRowsDirect(0, 100);
One row here would suffice?
Larger point: I imagine that the number of rows inserted during test setup doesn't actually matter. But, during code review, I look for patterns and try to spot differences. When one test inserts one row but the next inserts one hundred, I see the difference and instinctively think there's a good reason for it, and I start hunting for a code comment to explain. When I don't find one, I assume I'm missing something obvious and keep digging. If there's no actual reason for the difference (as I suspect is the case here), better to eliminate it to ensure no one reading through the code burns cycles trying to figure it out in the future.
Line 100: // TODO(awong): it would be nice to avoid getting an UNKNOWN_ERROR here.
Not understanding the TODO; we're asserting on TABLET_FAILED, not UNKNOWN_ERROR.
--
To view, visit http://gerrit.cloudera.org:8080/7441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] disk failure: local testing for disk failure
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7441
to look at the new patch set (#4).
Change subject: disk failure: local testing for disk failure
......................................................................
disk failure: local testing for disk failure
This patch adds tests that disk failures are handled as expected on a
single server. Various tablet operations (flushes, scans, compactions,
startup) are tested to ensure that an IOError is returned when
failure-injection is on. The server should not crash.
Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
---
M src/kudu/tserver/CMakeLists.txt
A src/kudu/tserver/ts_disk_failure-test.cc
2 files changed, 152 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/7441/4
--
To view, visit http://gerrit.cloudera.org:8080/7441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] disk failure: local testing for disk failure
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has abandoned this change. ( http://gerrit.cloudera.org:8080/7441 )
Change subject: disk failure: local testing for disk failure
......................................................................
Abandoned
Other tests subsumed testing of these disk failure code paths
--
To view, visit http://gerrit.cloudera.org:8080/7441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9c3461585711cd6f817c8c392d7967dc1d7e7be3
Gerrit-Change-Number: 7441
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins