Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24284 )
Change subject: IMPALA-14963: Query Profile AI Analysis Tool ...................................................................... Patch Set 16: (7 comments) http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/impala-http-handler.cc@204 PS16, Line 204: webserver->RegisterUrlCallback("/query_ai_analysis", "query_ai_analysis.tmpl", : MakeCallback(this, &ImpalaHttpHandler::QueryAiAnalysisHandler), false); : : webserver->RegisterUrlCallback("/query_ai_analysis_generate", "raw_text.tmpl", : MakeCallback(this, &ImpalaHttpHandler::QueryAiGenerateAnalysisHandler), false); Don't register these URLs if the query_profile_ai_analysis_max_concurrent_runs flag is 0. http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.h File be/src/service/query-profile-ai-analysis.h: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.h@42 PS16, Line 42: query_profile_ai_analysis_internal The convention in other files for these types of functions is to use a namespace named "test". Switch this namespace name and remove the "ForTest" suffix from the function names. http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@72 PS16, Line 72: DEFINE_validator(query_profile_ai_analysis_max_concurrent_runs, gt_zero); Typically, for these types of new features, we provide a way of turning them off. Thus, a value of 0 should completely disable the AI analysis functionality. http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@118 PS16, Line 118: int max_concurrent_runs Since this is an internal-only function, there is no need to pass in max_concurrent_runs. Instead, use the FLAGS_query_profile_ai_analysis_max_concurrent_runs variable directly on line 121. http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@119 PS16, Line 119: DCHECK_GT(max_concurrent_runs, 0); No need for this check as the flag validator ensures the number is greater than 0. http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@120 PS16, Line 120: int current_runs = num_profiles_being_analyzed.load(std::memory_order_relaxed); : while (current_runs < max_concurrent_runs) { : if (num_profiles_being_analyzed.compare_exchange_weak(current_runs, : current_runs + 1, std::memory_order_acq_rel, std::memory_order_relaxed)) { : return true; : } : } : return false; Since multiple operations need to happen to determine if the max number of profiles is exceeded, a mutex would be cleaner. static mutex num_profiles_being_analyzed_lock; static int8_t num_profiles_being_analyzed = 0; static bool TryAcquireAiAnalysisSlot() { lock_guard<mutex> l(num_profiles_being_analyzed_lock); if (current_runs >= max_concurrent_runs) { return false; } num_profiles_being_analyzed++; return true; } http://gerrit.cloudera.org:8080/#/c/24284/16/be/src/service/query-profile-ai-analysis.cc@131 PS16, Line 131: const int previous_runs = num_profiles_being_analyzed.fetch_sub( Will need to hold the num_profiles_being_analyzed_lock mutex here. -- To view, visit http://gerrit.cloudera.org:8080/24284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9376b8c56df75032f51abbb76b3ac6927f9107e Gerrit-Change-Number: 24284 Gerrit-PatchSet: 16 Gerrit-Owner: Gokul Kolady <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Gokul Kolady <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 20 May 2026 02:34:03 +0000 Gerrit-HasComments: Yes
