Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21142 )
Change subject: IMPALA-12737: Query columns in workload management tables. ...................................................................... Patch Set 42: (13 comments) http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-flags.cc File be/src/service/workload-management-flags.cc: http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-flags.cc@174 PS40, Line 174: workload_mgmt_schema_version, "1.1.0" > Question: if there is a bug exist in a specific version (mistake in collect No, it is not possible to downgrade. Upgrades are one-way only. I'm going to investigate adding a flag to override the latest schema version though specifically for this case where a bug exists in the workload management code. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc File be/src/service/workload-management-init.cc: http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@70 PS40, Line 70: > This shouldn't need to be a shared_ptr. It's not held beyond the lifetime o Very good suggestion. I switched the class level internal_server_ variable from a shared_ptr<InternalServer> to InternalServer* since ownership is never shared or transferred. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@103 PS40, Line 103: > Doesn't need to be a shared_ptr. Done http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@152 PS40, Line 152: > Doesn't need to be a shared_ptr. Done http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@354 PS40, Line 354: // concurrently on all coordinators. Running the same create and alter table statements > Could we do all this setup in catalogd instead? Trying to avoid errors by j It's not my favorite approach either. I've looked some into using the catalog, but that route requires implementing a completely new concept in the catalog which I hesitate to do in the catalog. I will look some more into that and will also think more if there is a deterministic way we can determine how long each coordinator sleeps. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@387 PS40, Line 387: if (auto v = KNOWN_VERSIONS.find(target_schema_version); v == KNOWN_VERSIONS.end()) { > Might be nice to list the known versions. Done http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management.cc@210 PS40, Line 210: } > It's not clear to me if this needs to be held through the whole function. C Good question, the need for this mutex is important for coordinating the shutdown process to ensure all queued completed queries are inserted into the query log table before the coordinator process ends. The ImpalaServer::ShutdownWorkloadManagement function in workload-management.cc uses a condition_variable to block the coordinator shutdown process while the completed queries queue is drained. Additionally, thus mutex ensures no invalid state transitions and no data corruption occurs during the situation where coordinator startup is interrupted by a shutdown. I did some refactoring on the namings to clarify their purposes and also found a bug where the shutdown process was not releasing its lock before notifying the condition_variable. http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/Expr.java@2041 PS40, Line 2041: recordChildrenInWork > 'isCandidateForQueryLog' might fit better. Agreed this function name is not clear enough. Renamed. http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@380 PS40, Line 380: registerReferencedColumns(); > Why guard it? They're also added to the profile. Good point, I had forgotten about the profile. http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@387 PS40, Line 387: selectList_.getItems().stream().filter(elem -> !elem.isStar()) > Why are column aliases not resolved? Good catch, that is an out-of-date comment and has been removed. http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/test/java/org/apache/impala/planner/ColumnsTest.java File fe/src/test/java/org/apache/impala/planner/ColumnsTest.java: http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/test/java/org/apache/impala/planner/ColumnsTest.java@68 PS40, Line 68: testColumns(query, "default", select, where, join, aggregate, orderBy); > typo: junit Done http://gerrit.cloudera.org:8080/#/c/21142/40/tests/custom_cluster/test_workload_mgmt_init.py File tests/custom_cluster/test_workload_mgmt_init.py: http://gerrit.cloudera.org:8080/#/c/21142/40/tests/custom_cluster/test_workload_mgmt_init.py@165 PS40, Line 165: def test_create_on_version_1_1_0(self): > This could be merged into test_upgrade_1_0_0_to_1_1_0. It's less important Done http://gerrit.cloudera.org:8080/#/c/21142/40/tests/custom_cluster/test_workload_mgmt_sql_details.py File tests/custom_cluster/test_workload_mgmt_sql_details.py: http://gerrit.cloudera.org:8080/#/c/21142/40/tests/custom_cluster/test_workload_mgmt_sql_details.py@226 PS40, Line 226: ["warehouse.w_warehouse_name", "ship_mode.sm_type", "web_site.web_name"], > typo: additional Done -- To view, visit http://gerrit.cloudera.org:8080/21142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78f3670b067c0c192ee8a212fba95466fbcb51d7 Gerrit-Change-Number: 21142 Gerrit-PatchSet: 42 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 29 Aug 2024 18:47:44 +0000 Gerrit-HasComments: Yes
