Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )
Change subject: IMPALA-10268: Validate the debug actions when they are set ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/service/query-options.cc@235 PS4, Line 235: VerifyExecNodeDeb > Rename to VerifyExecNodeDebugAction. Good idea, ack. http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/22734/4/be/src/util/debug-util.cc@459 PS4, Line 459: if (!verify_only) { : DCHECK(false) << "Invalid debug action"; : } : return Status(Substitute(ERROR_MSG, components[0], action_str, "invalid command")); > Sorry, should be like this. This can be done, but it would require some cross referencing: query-options.cc::SetQueryOption() -> debug-util.cc::DebugActionImpl() -> query-options.cc::VerifyExecNodeDebugAction(). But more generally, I'd really like to keep General and ExecNode debug actions independent, as they are in the code, and not call one from another. Absolutely General and ExecNode parsing can be implemented better together, handling all corner cases, but as I describe in commit message, imho that work would better be done in scope of more general refactoring to avoid kind of half-way solution. -- To view, visit http://gerrit.cloudera.org:8080/22734 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I53816aba2c79b556688d3b916883fee7476fdbb5 Gerrit-Change-Number: 22734 Gerrit-PatchSet: 5 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Tue, 22 Apr 2025 09:55:22 +0000 Gerrit-HasComments: Yes
