Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22734 )
Change subject: WIP IMPALA-10268: Validate the debug actions when they are set ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.h@353 PS3, Line 353: static Status VerifyDebugAction(const TDebugOptions& debug_options); Move this to query-options.h? http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/exec/exec-node.cc@466 PS3, Line 466: if (!debug_options_.action_param.empty()) { : RETURN_IF_ERROR( : ParseAndValidateSleepDuration(debug_options_.action_param, &sleep_duration_ms)); : } This can be removed if ExecNode::VerifyDebugAction() is called right after L438. It also make sense to do so, in case VerifyDebugAction is expanded to validate other things later. http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307 PS3, Line 307: Status status = DebugActionVerifyOnly(value); : if (!status.ok() && string::npos == status.msg().msg().find("invalid command")){ : return status; : } Why not simply this? RETURN_IF_ERROR(DebugActionVerifyOnly(value)); http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307 PS3, Line 307: Status status = DebugActionVerifyOnly(value); : if (!status.ok() && string::npos == status.msg().msg().find("invalid command")){ : return status; : } : DebugOptions debug_options(value); : RETURN_IF_ERROR(ExecNode::VerifyDebugAction(debug_options.ToThrift())); Please leave a short comment on why validation need to go through 2 different functions: DebugActionVerifyOnly and VerifyDebugAction. http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.h@147 PS3, Line 147: /// Slow path implementing DebugAction() for the case where 'debug_action' is non-empty. Please document what is the impact of verify_only value. http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/util/debug-util.cc@366 PS3, Line 366: if (!verify_only) { nit: I think this will be better understood if the condition is flipped. if (verify_only) { // Only validate the action_str going forward. ... } else { ... } -- 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: 3 Gerrit-Owner: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Mon, 14 Apr 2025 19:24:02 +0000 Gerrit-HasComments: Yes
