Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24502 )
Change subject: IMPALA-15107: Support Direct Thrift Upload for AI Profile Analyzer ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/service/impala-http-handler.cc@237 PS6, Line 237: Webserver::RawUrlCallback query_ai_analysis_generate_callback = : [this](const auto& req, auto* data, auto* response) { : return this->QueryAiGenerateAnalysisHandler(req, data, response); : }; Is there a specific need for this separate query_ai_analysis_generate_callback variable? If not, please eliminate it. http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/service/query-profile-ai-analysis-test.cc File be/src/service/query-profile-ai-analysis-test.cc: http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/service/query-profile-ai-analysis-test.cc@145 PS6, Line 145: TEST(QueryProfileAiAnalysisHelpersTest, ConvertThriftProfileArchiveToJson) { It would be good to have a happy path test case that uses an actual binary Thrift profile and asserts the correct json is returned. http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/service/query-profile-ai-analysis.cc File be/src/service/query-profile-ai-analysis.cc: http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/service/query-profile-ai-analysis.cc@920 PS6, Line 920: ObjectPool profile_pool; There is the potential here for a denial of service attack if the client uploads more data than the process can hold in memory. A couple of things are needed here: 1. Hard limit on uploaded profile size, I suggest making this value a configurable startup flag that defaults to 500mb. 2. Use the process-level mem tracker to ensure there is enough memory available to hold the entire profile. http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/24502/6/be/src/util/runtime-profile.h@746 PS6, Line 746: static Status CreateFromArchiveString(const std::string& archive_str, ObjectPool* pool, : RuntimeProfile** out); It should be possible to eliminate this function overload. -- To view, visit http://gerrit.cloudera.org:8080/24502 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iff05c3cde7b34a56e31f020611cc33ff4e02aa32 Gerrit-Change-Number: 24502 Gerrit-PatchSet: 6 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: Quanlong Huang <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 11 Sep 2026 14:56:21 +0000 Gerrit-HasComments: Yes
