Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10690 )
Change subject: IMPALA-7046: introduce "global" debug_actions ...................................................................... Patch Set 8: (9 comments) Thanks for the reviews Tim and Bikram! http://gerrit.cloudera.org:8080/#/c/10690/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10690/8//COMMIT_MSG@28 PS8, Line 28: of Release builds. > Your approach to this changed, right? Done http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/runtime/debug-options.cc File be/src/runtime/debug-options.cc: http://gerrit.cloudera.org:8080/#/c/10690/6/be/src/runtime/debug-options.cc@58 PS6, Line 58: continue > I think all global debug actions have a size of exactly 2 components (separ That's true - my initial implementation had three components for global actions. I'll revert this and add a comment instead. http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/runtime/debug-options.cc File be/src/runtime/debug-options.cc: http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/runtime/debug-options.cc@60 PS8, Line 60: DCHECK(!(phase_ == TExecNodePhase::CLOSE && action_ == TDebugAction::WAIT)) > Maybe we should just log an error here - I don't think we want users to be Yeah, I was thinking that too but then decided to just copy the logic from old line 60, since I wanted to return an error status but that's a bit non-trivial right now. I'll fix it up a bit now to do the logging. http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@295 PS8, Line 295: list > Any reason to use a list and not a vector? Seems arbitrary but I might be m Just figured you don't really need random access to the list of debug actions and there is no ordering of that thing. That's unlikely the components of each debug action, where you do want random access and ordering is meaningful, so vector there. http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@318 PS8, Line 318: if (parse_result != StringParser::PARSE_SUCCESS || > Should we log a parse failure? It turns into a status at lines 356/367. Are you looking for something more? http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@323 PS8, Line 323: *should_execute = rand() < probability * (RAND_MAX + 1L); > We use rand() here and std::mt19937 below. Any reason not to standardise on Not really. I think I'll change the one below to rand() which is what I meant to do but forgot (because it seems wrong to create a new random device each time). http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@331 PS8, Line 331: error_msg > How about upper case to make it clearer that it's a constant? I was momenta Done http://gerrit.cloudera.org:8080/#/c/10690/8/be/src/util/debug-util.cc@339 PS8, Line 339: if (cmd.compare("SLEEP") == 0) { > This is ok, but why not (cmd == "SLEEP")? Dislike for operator overloading? yeah, I do generally disklike operator overloading, though don't feel too strongly in this particular case. also carried this from the old code at line 293. I'f you don't feel strongly, I'd prefer to leave it. http://gerrit.cloudera.org:8080/#/c/10690/8/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/10690/8/common/thrift/ImpalaService.thrift@91 PS8, Line 91: // 2. Global actions > I think this is fine for now but if the set of things continue to grow I wo Yeah, let's defer that. I'm not sure there are that many more commands we need. -- To view, visit http://gerrit.cloudera.org:8080/10690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77663a539be18711a4f12c470ffd7474e3d69388 Gerrit-Change-Number: 10690 Gerrit-PatchSet: 8 Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 19 Jun 2018 17:51:16 +0000 Gerrit-HasComments: Yes
