Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19214 )
Change subject: IMPALA-7969: Always admit trivial queries immediately ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/19214/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/19214/2/be/src/scheduling/admission-controller.cc@340 PS2, Line 340: ss << " local_host(local_mem_admitted=" << PrintBytes(local_mem_admitted_) << ", "; local_trivial_running_ could be added to the debug string http://gerrit.cloudera.org:8080/#/c/19214/2/be/src/scheduling/admission-controller.cc@784 PS2, Line 784: LOG(INFO) << "Release query id " << query_id; Is this log line intentional? http://gerrit.cloudera.org:8080/#/c/19214/2/be/src/scheduling/admission-controller.cc@1914 PS2, Line 1914: << " max_mem=" << PrintBytes(max_mem); Maybe add state->GetIsTrivialQuery() to the log? http://gerrit.cloudera.org:8080/#/c/19214/2/be/src/scheduling/admission-controller.cc@1921 PS2, Line 1921: Admiited typo http://gerrit.cloudera.org:8080/#/c/19214/2/fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java File fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java: http://gerrit.cloudera.org:8080/#/c/19214/2/fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java@22 PS2, Line 22: import org.apache.impala.analysis.Expr; nit: usually there is a blank line betwwen java. and org. imports http://gerrit.cloudera.org:8080/#/c/19214/2/fe/src/main/java/org/apache/impala/planner/TrivialQueryChecker.java@82 PS2, Line 82: if (!queryOptions.isEnable_trivial_query_for_admission() || !isQueryStmt) nit: we usually add braces to the block when breaking a line http://gerrit.cloudera.org:8080/#/c/19214/2/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/19214/2/tests/custom_cluster/test_admission_controller.py@1216 PS2, Line 1216: "select 1 union all select sleep(10000)", False) Can you check what happens when the sleep is after an empty set? e.g. select 1 from functional.alltypes limit 0 union all select sleep(1) another query I am curious about is select a from (select 1 a, sleep(1)) s; http://gerrit.cloudera.org:8080/#/c/19214/2/tests/custom_cluster/test_session_expiration.py File tests/custom_cluster/test_session_expiration.py: http://gerrit.cloudera.org:8080/#/c/19214/2/tests/custom_cluster/test_session_expiration.py@107 PS2, Line 107: queued_handle = client.execute_async("select sleep(1)") I think the test would be clearer with keeping select 1, but setting enable_trivial_query_for_admission to false. http://gerrit.cloudera.org:8080/#/c/19214/2/tests/custom_cluster/test_shell_interactive.py File tests/custom_cluster/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/19214/2/tests/custom_cluster/test_shell_interactive.py@51 PS2, Line 51: roc.sendline("select sleep(1);") I think the test would be clearer with keeping select 1, but setting enable_trivial_query_for_admission to false. Currently it is not evident why sleep is used. -- To view, visit http://gerrit.cloudera.org:8080/19214 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a729764e3055d7eb11900c96c82ff53eb261f91 Gerrit-Change-Number: 19214 Gerrit-PatchSet: 2 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Mon, 02 Jan 2023 12:45:38 +0000 Gerrit-HasComments: Yes
