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 4: (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: MemPool* expr_perm_pool() { return expr_perm_pool_.get(); } > Move this to query-options.h? Agree, moved. 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: RETURN_IF_ERROR( : ParseAndValidateSleepDuration(debug_options_.action_param, &sleep_duration_ms)); : VLOG(1) << "DEBUG_ACTION: Sleeping for " << sleep_duration_ms << "ms"; : S > This can be removed if ExecNode::VerifyDebugAction() is called right after I moved 'VerifyDebugAction()' to query-options.h as proposed in another thread, and refactored it a bit. So we probably don't want to have cross references from query-options.h here and back. 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: break; : } : case TImpalaQueryOptions::NUM_SCANNER_THREADS: { : i > Why not simply this? 'value' string may contain both General and ExecNode debug actions. 'DebugActionVerifyOnly' checks for General ones and in case of error status we return. But if the error is just 'invalid command' it's possible that there's an ExecNode which needs to be verified below in 'ExecNode::VerifyDebugAction'. It's not a perfect logic still, but perhaps the best we can do given independent / different implementations of two debug option types. http://gerrit.cloudera.org:8080/#/c/22734/3/be/src/service/query-options.cc@307 PS3, Line 307: break; : } : case TImpalaQueryOptions::NUM_SCANNER_THREADS: { : int32_t int32_t_val = 0; : RETURN_IF_ERROR(QueryOptionParser::Parse<int32_t>(option, value, &int32_t_val)); : query_options->__set_num_scanner_threads(int32_t_val); > Please leave a short comment on why validation need to go through 2 differe Done 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. Ack 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. Hmm, on the other hand this way it's more consistent with the checks below, all of which are 'if (!verify_only)'. If you don't insist I would keep it. -- 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: 4 Gerrit-Owner: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Wed, 16 Apr 2025 13:23:31 +0000 Gerrit-HasComments: Yes
