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