Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20762 )
Change subject: IMPALA-12540: Query Live Table ...................................................................... Patch Set 41: (8 comments) http://gerrit.cloudera.org:8080/#/c/20762/41/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/20762/41/be/src/service/fe-support.cc@655 PS41, Line 655: for (const auto& it : membership_snapshot->current_backends) { : if (it.second.has_is_coordinator() && it.second.is_coordinator()) { : VLOG_QUERY << "Found coordinator " : << it.second.address().hostname() << ":" << it.second.address().port(); : coordinators.emplace_back(FromNetworkAddressPB(it.second.address())); : } : } What do you think about pushing this to ClusterMembershipMgr or ClusterMembershipMgr::Snapshot? That will allow adding BE test in scheduler-test.cc / scheduler-test-util.cc http://gerrit.cloudera.org:8080/#/c/20762/41/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/20762/41/common/thrift/CatalogObjects.thrift@672 PS41, Line 672: QUERY_LIVE > This pattern gives flexibility to add more system tables in the future shou Ack, In that case, please assign cardinal number. QUERY_LIVE = 0 Also add comment on where to find the actual name string. http://gerrit.cloudera.org:8080/#/c/20762/41/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: http://gerrit.cloudera.org:8080/#/c/20762/41/common/thrift/StatestoreService.thrift@207 PS41, Line 207: // A list of network addresses : struct TAddressesList { : 1: required list<Types.TNetworkAddress> addresses; : } Move this to Types.thrift? I understand this is required to serialize response for NativeGetCoordinators(). http://gerrit.cloudera.org:8080/#/c/20762/41/common/thrift/SystemTables.thrift File common/thrift/SystemTables.thrift: http://gerrit.cloudera.org:8080/#/c/20762/41/common/thrift/SystemTables.thrift@21 PS41, Line 21: enum TQueryColumn { nit: I think TSystemTableColumn is a better fit. Please also put comment that these columns are all currently for QUERY_LIVE / impala_query_live table. http://gerrit.cloudera.org:8080/#/c/20762/41/fe/src/main/java/org/apache/impala/catalog/SystemTable.java File fe/src/main/java/org/apache/impala/catalog/SystemTable.java: http://gerrit.cloudera.org:8080/#/c/20762/41/fe/src/main/java/org/apache/impala/catalog/SystemTable.java@52 PS41, Line 52: private static Map<String, TSystemTableName> SYSTEM_TABLE_NAME_MAP = : ImmutableMap.of(QUERY_LIVE, TSystemTableName.QUERY_LIVE); Consider using ImmutableMap. Please also add FE test to assert the key-value pair here. Maybe something like in FileSystemUtilTest.java. http://gerrit.cloudera.org:8080/#/c/20762/41/fe/src/main/java/org/apache/impala/catalog/SystemTable.java@176 PS41, Line 176: : private static org.apache.hadoop.hive.metastore.api.Table : createMetastoreTable(String dbName, String tableName, String owner) { > Right, it doesn't send anything to HMS. This is an Impala-only table, that Ack http://gerrit.cloudera.org:8080/#/c/20762/41/fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java File fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java: http://gerrit.cloudera.org:8080/#/c/20762/41/fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java@121 PS41, Line 121: 1024L * 1024L > The completed queries table cuts off the SQL and PLAN fields based on coord Looks like 1MB is a standard MIN_MEMORY_ESTIMATE. https://github.com/apache/impala/blob/814acf3bbf015ca305603d33dada8f4233d929d0/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java#L148-L152 With that consideration, I'm okay with leaving this as is. We can refine later if we found an issue. http://gerrit.cloudera.org:8080/#/c/20762/41/tests/custom_cluster/test_query_live.py File tests/custom_cluster/test_query_live.py: http://gerrit.cloudera.org:8080/#/c/20762/41/tests/custom_cluster/test_query_live.py@27 PS41, Line 27: @CustomClusterTestSuite.with_args(impalad_args="--enable_workload_mgmt " : "--cluster_id=test_query_live_1", : catalogd_args="--enable_workload_mgmt") > sys.impala_query_live is currently only accessible if workload management i Ack -- To view, visit http://gerrit.cloudera.org:8080/20762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2f9a449f0e5502078931e7f1c5df6e0b762c743 Gerrit-Change-Number: 20762 Gerrit-PatchSet: 41 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Andrew Sherman <[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, 20 Mar 2024 20:05:05 +0000 Gerrit-HasComments: Yes
