[kudu-CR] disk failure: local testing for disk failure

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] disk failure: local testing for disk failure

[Andrew Wong (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] disk failure: local testing for disk failure

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] disk failure: local testing for disk failure

[Andrew Wong (Code Review)]

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