[kudu-CR] Add fault injection of EIOs
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/6881 Change subject: Add fault injection of EIOs .. Add fault injection of EIOs This patch adds the functionality to inject EIOs in env_posix based on a flag-specified ECMAScript regex determining which paths should fail and a flag-specified double indicating how often these failures should occur. Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 --- M src/kudu/util/env.h M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h 4 files changed, 97 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6881/1 -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] Add fault injection of EIOs
Andrew Wong has uploaded a new patch set (#2). Change subject: Add fault injection of EIOs .. Add fault injection of EIOs This patch adds the functionality to inject EIOs in env_posix based on a flag-specified ECMAScript regex determining which paths should fail and a flag-specified double indicating how often these failures should occur. Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 --- M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h 3 files changed, 93 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6881/2 -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add fault injection of EIOs
Andrew Wong has posted comments on this change. Change subject: Add fault injection of EIOs .. Patch Set 2: For now I've left the injection in env_posix.cc since it's only used within this class. If other files can take advantage of the functionality, I'll gladly move it. -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] Add fault injection of EIOs
David Ribeiro Alves has posted comments on this change. Change subject: Add fault injection of EIOs .. Patch Set 2: (5 comments) a small directed test would be nice. maybe just add a test to env_util test where you set the prob flag to 1 and make sure the methods you care about return what you want. http://gerrit.cloudera.org:8080/#/c/6881/2/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: PS2, Line 129: the operation should attempt to fail without crashing maybe "If false, EIOs are just cause an Status::IOError to be returned. In this case, it's up to the caller to decide how to handle the error." Line 149: extra line PS2, Line 150: env_inject_eio any operation or just the ones that whose filenames match the regex below? PS2, Line 283: WARNING use ERROR instead? Line 1651: spurious change -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] Add fault injection of EIOs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6881 to look at the new patch set (#3). Change subject: Add fault injection of EIOs .. Add fault injection of EIOs This patch adds the functionality to inject EIOs in env_posix based on a flag-specified ECMAScript regex determining which paths should fail and a flag-specified double indicating how often these failures should occur. Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 --- M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h 3 files changed, 93 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6881/3 -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] Add fault injection of EIOs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6881 to look at the new patch set (#4). Change subject: Add fault injection of EIOs .. Add fault injection of EIOs This patch adds the functionality to inject EIOs in env_posix based on a flag-specified ECMAScript regex determining which paths should fail and a flag-specified double indicating how often these failures should occur. A test is added to env-test.cc to verify that EIOs are successfully triggered by specifying regexes. Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 --- M src/kudu/util/env-test.cc M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h 4 files changed, 131 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6881/4 -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] Add fault injection of EIOs
Andrew Wong has posted comments on this change.
Change subject: Add fault injection of EIOs
..
Patch Set 4:
(25 comments)
http://gerrit.cloudera.org:8080/#/c/6881/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
PS2, Line 129: error-handling up to the caller.");
> maybe "If false, EIOs are just cause an Status::IOError to be returned. In
Done.
Line 149:
> extra line
Done
PS2, Line 150: env_inject_eio
> any operation or just the ones that whose filenames match the regex below?
Is it okay for flag descriptions to refer to "above" or "below," wrt code
placement?
For now, I'll keep it vague and say "operations on certain files" instead of
"any."
PS2, Line 283: ERROR)
> use ERROR instead?
Done
Line 290: MAYBE_INJECT_EIO(filename, IOError(Env::kInjectedFailureStatusMsg,
EIO));
> warning: local copy 'f_' of the variable 'filename' is never modified; cons
Done
Line 311: MAYBE_INJECT_EIO(filename, IOError(Env::kInjectedFailureStatusMsg,
EIO));
> warning: local copy 'f_' of the variable 'filename' is never modified; cons
Done
Line 335: MAYBE_INJECT_EIO(filename, IOError(Env::kInjectedFailureStatusMsg,
EIO));
> warning: local copy 'f_' of the variable 'filename' is never modified; cons
Done
Line 404: MAYBE_INJECT_EIO(filename, IOError(Env::kInjectedFailureStatusMsg,
EIO));
> warning: local copy 'f_' of the variable 'filename' is never modified; cons
Done
Line 953: ThreadRestrictions::AssertIOAllowed();
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 973: ThreadRestrictions::AssertIOAllowed();
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 1044: ThreadRestrictions::AssertIOAllowed();
> warning: local copy 'f_' of the variable 'dir' is never modified; consider
Done
Line 1062: ThreadRestrictions::AssertIOAllowed();
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 1072: TRACE_EVENT1("io", "PosixEnv::CreateDir", "path", name);
> warning: local copy 'f_' of the variable 'name' is never modified; consider
Done
Line 1083: TRACE_EVENT1("io", "PosixEnv::DeleteDir", "path", name);
> warning: local copy 'f_' of the variable 'name' is never modified; consider
Done
Line 1107: TRACE_EVENT1("io", "PosixEnv::ChangeDir", "dest", dest);
> warning: local copy 'f_' of the variable 'dest' is never modified; consider
Done
Line 1118: TRACE_EVENT1("io", "SyncDir", "path", dirname);
> warning: local copy 'f_' of the variable 'dirname' is never modified; consi
Done
Line 1139: TRACE_EVENT1("io", "PosixEnv::GetFileSize", "path", fname);
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 1153: TRACE_EVENT1("io", "PosixEnv::GetFileSizeOnDisk", "path", fname);
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 1183: TRACE_EVENT1("io", "PosixEnv::GetBlockSize", "path", fname);
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 1197: TRACE_EVENT1("io", "PosixEnv::GetFileModifiedTime", "fname",
fname);
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 1215: ThreadRestrictions::AssertIOAllowed();
> warning: local copy 'f_' of the variable 'path' is never modified; consider
Done
Line 1235: MAYBE_INJECT_EIO(target, IOError(Env::kInjectedFailureStatusMsg,
EIO));
> warning: local copy 'f_' of the variable 'src' is never modified; consider
Done
Line 1236: TRACE_EVENT2("io", "PosixEnv::RenameFile", "src", src, "dst",
target);
> warning: local copy 'f_' of the variable 'target' is never modified; consid
Done
Line 1247: TRACE_EVENT1("io", "PosixEnv::LockFile", "path", fname);
> warning: local copy 'f_' of the variable 'fname' is never modified; conside
Done
Line 1651:
> spurious change
Done
--
To view, visit http://gerrit.cloudera.org:8080/6881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Add fault injection of EIOs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6881 to look at the new patch set (#5). Change subject: Add fault injection of EIOs .. Add fault injection of EIOs This patch adds the functionality to inject EIOs in env_posix based on a flag-specified ECMAScript regex determining which paths should fail and a flag-specified double indicating how often these failures should occur. A test is added to env-test.cc to verify that EIOs are successfully triggered by specifying regexes. Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 --- M src/kudu/util/env-test.cc M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h 4 files changed, 133 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6881/5 -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] Add fault injection of EIOs
Adar Dembo has posted comments on this change.
Change subject: Add fault injection of EIOs
..
Patch Set 8:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/6881/8/src/kudu/util/env-test.cc
File src/kudu/util/env-test.cc:
Line 1009: FLAGS_env_bad_dir_regex = Substitute("($0)", kTestRWPath1);
Why do we need to surround the pathname with parens?
Line 1016: FLAGS_env_bad_dir_regex = "(neither_path)";
And here too?
http://gerrit.cloudera.org:8080/#/c/6881/8/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 107: #define MAYBE_INJECT_EIO(filename_expr, error_expr) do { \
Can we call this MAYBE_RETURN_EIO to emphasize the similarity with
MAYBE_RETURN_FAILURE?
Line 150: DEFINE_double(env_inject_eio, 0.0,
Why can't we reuse env_inject_io_error? I think it's reasonable for
env_inject_io_error to always add EIO posix codes, if that would help.
Line 153: DEFINE_string(env_bad_dir_regex, "",
Should rename to emphasize symmetry with env_inject_eio/env_inject_io_error
(i.e. "env_inject_eio_dir_regex" or some such).
Line 154: "ECMAScript regex specifying bad directories. If empty,
all "
Nit: Instead of "bad" just explain how this ties into error injection.
Line 156: TAG_FLAG(env_inject_eio, hidden);
Nit: it's easy to tag the wrong flags, so let's colocate the flag tags and
definitions (i.e. don't have the definitions first and tags next).
http://gerrit.cloudera.org:8080/#/c/6881/8/src/kudu/util/fault_injection.h
File src/kudu/util/fault_injection.h:
Line 46: // Note that 'status_expr' is evaluated even if 'fraction_flag' is
zero.
Can this be addressed? Seems like you could use MaybeTrue() on fraction_flag
now, before expanding status_eval?
PS8, Line 51: desribed
described
--
To view, visit http://gerrit.cloudera.org:8080/6881
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Add fault injection of EIOs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6881 to look at the new patch set (#11). Change subject: Add fault injection of EIOs .. Add fault injection of EIOs This patch adds the functionality to inject EIOs in env_posix based on a flag-specified ECMAScript regex determining which paths should fail and a flag-specified double indicating how often these failures should occur. A test is added to env-test.cc to verify that EIOs are successfully triggered by specifying regexes. Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 --- M src/kudu/util/env-test.cc M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h 4 files changed, 132 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6881/11 -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add fault injection of EIOs
Andrew Wong has posted comments on this change. Change subject: Add fault injection of EIOs .. Patch Set 11: (7 comments) Haven't addressed everything, marked some "will do"s. Pushed after rebasing as along with the EMC patch http://gerrit.cloudera.org:8080/#/c/6881/8/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 107: const string& f_ = (filename_expr); \ > Can we call this MAYBE_RETURN_EIO to emphasize the similarity with MAYBE_RE Done Line 150: "Fraction of the time that operations on certain files will fail " > Why can't we reuse env_inject_io_error? I think it's reasonable for env_inj We can, although there's the subtlety that suicide_on_eio should be switched to false in tests that use the current env_inject_io_error. Will try it out. Line 153: DEFINE_string(env_inject_eio_regex, "", > Should rename to emphasize symmetry with env_inject_eio/env_inject_io_error Done Line 154: "ECMAScript regex specifying files on which I/O will fail. If " > Nit: Instead of "bad" just explain how this ties into error injection. Done Line 156: TAG_FLAG(env_inject_eio_regex, hidden); > Nit: it's easy to tag the wrong flags, so let's colocate the flag tags and Done http://gerrit.cloudera.org:8080/#/c/6881/8/src/kudu/util/fault_injection.h File src/kudu/util/fault_injection.h: Line 46: // Note that 'status_expr' is evaluated even if 'fraction_flag' is zero. > Can this be addressed? Seems like you could use MaybeTrue() on fraction_fla Yeah that's MAYBE_EVAL_AND_RETURN_FAILURE. Will replace the implementation since it seems like MAYBE_EVAL_AND_RETURN_FAILURE's behavior is expected. PS8, Line 51: describe > described Done -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Add fault injection of EIOs
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6881 to look at the new patch set (#12). Change subject: Add fault injection of EIOs .. Add fault injection of EIOs This patch adds the functionality to inject EIOs in env_posix based on a flag-specified glob pattern indicating which paths should fail and a flag-specified double indicating how often these failures should occur. A test is added to env-test.cc to verify that EIOs are successfully triggered by specifying the patterns. Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 --- M src/kudu/util/env-test.cc M src/kudu/util/env_posix.cc M src/kudu/util/fault_injection.cc M src/kudu/util/fault_injection.h 4 files changed, 140 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6881/12 -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add fault injection of EIOs
Adar Dembo has posted comments on this change. Change subject: Add fault injection of EIOs .. Patch Set 12: (3 comments) http://gerrit.cloudera.org:8080/#/c/6881/12//COMMIT_MSG Commit Message: Line 10: a flag-specified glob pattern indicating which paths should fail What made you decide to use globbing instead of regexes? I don't have an opinion one way or the other, but am curious to know the motivation for the change. http://gerrit.cloudera.org:8080/#/c/6881/8/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 150: TAG_FLAG(env_inject_eio, hidden); > We can, although there's the subtlety that suicide_on_eio should be switche As of PS12 I see still see both env_inject_io_error and env_inject_eio. Are you still working on combining them? http://gerrit.cloudera.org:8080/#/c/6881/8/src/kudu/util/fault_injection.h File src/kudu/util/fault_injection.h: Line 46: // Note that 'status_expr' is evaluated even if 'fraction_flag' is zero. > Yeah that's MAYBE_EVAL_AND_RETURN_FAILURE. Will replace the implementation Are you still working on this? -- To view, visit http://gerrit.cloudera.org:8080/6881 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5190d5e7f296cf27b65f1ecd2c16f3315cc46a39 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
