Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21142 )
Change subject: IMPALA-12737: Query columns in workload management tables. ...................................................................... Patch Set 40: (12 comments) 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: static void _setupDb(shared_ptr<InternalServer> internal_server, This shouldn't need to be a shared_ptr. It's not held beyond the lifetime of the function call. You're just adding overhead and artificially constraining the function call. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@103 PS40, Line 103: static void _setupTable(shared_ptr<InternalServer> internal_server, Doesn't need to be a shared_ptr. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@152 PS40, Line 152: static void _upgradeTable(shared_ptr<InternalServer> internal_server, Doesn't need to be a shared_ptr. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@328 PS40, Line 328: VLOG(2) << "Dropping '" << live_table_name << "' so it can be upgraded to " Note to self: I still want to look at why we can't alter this table. I was pretty sure I ran tests that verified we could. If we could fix that, then _logTableSchemaManagement and _liveTableSchemaManagement would be almost identical. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@354 PS40, Line 354: // errors when running the alter table statements. Could we do all this setup in catalogd instead? Trying to avoid errors by jitter is not an ideal approach. http://gerrit.cloudera.org:8080/#/c/21142/40/be/src/service/workload-management-init.cc@387 PS40, Line 387: FLAGS_workload_mgmt_schema_version, "' does not match any known versions")); Might be nice to list the known versions. 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: lock_guard<mutex> l(workload_mgmt_threadstate_mu_); It's not clear to me if this needs to be held through the whole function. Could workload_mgmt_thread_state_ be an atomic instead? 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: if (BackendConfig.INSTANCE.enableWorkloadMgmt()) { Why guard it? They're also added to the profile. http://gerrit.cloudera.org:8080/#/c/21142/40/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@387 PS40, Line 387: // Joins will be added during planning. Column aliases are not resolved. Why are column aliases not resolved? 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: // Runs a junt test against the default db. typo: junit 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_0_0(self): This could be merged into test_upgrade_1_0_0_to_1_1_0. It's less important that Impala is able to start up with an older version except in the context of upgrade testing. 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: Query 64 tests addtional subqueries functionality.""" typo: additional -- 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: 40 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: Wed, 28 Aug 2024 21:27:21 +0000 Gerrit-HasComments: Yes
