Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/20762 )
Change subject: IMPALA-12540: Add system.impala_query_live table ...................................................................... Patch Set 28: (14 comments) http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.h File be/src/exec/system-table-scanner.h: http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.h@24 PS28, Line 24: struct QueryStateExpanded; Why take this approach instead of importing query-state-record.h? http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc File be/src/exec/system-table-scanner.cc: http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@19 PS28, Line 19: Please #include <memory> since make_unique<> is used http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@57 PS28, Line 57: Unknown table type Nit: a message of "Unknown table name" would be clearer. http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@57 PS28, Line 57: Substitute The docs on this constructor says it should not be used for status that are client visible. I think the TErrorCode::NOT_IMPLEMENTED_ERROR code is most appropriate to use here. https://github.com/apache/impala/blob/ca3fe6d6af6f5216f75bea26d6e90cf5cc816efc/be/src/common/status.h#L159-L162 http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@147 PS28, Line 147: if (boost::algorithm::iequals(name, event.first)) { Since this file does "using namespace boost::algorithm", can the "boost::algorithm::" prefix by dropped? http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/exec/system-table-scanner.cc@299 PS28, Line 299: && boost::algorithm::iends_with(event.first, " started")) { Since this file does "using namespace boost::algorithm", can all the "boost::algorithm::" prefixes by dropped? http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/fe-support.cc@651 PS28, Line 651: // TODO: should we modify SendClusterMembershipToFrontend instead? Note: TODO http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/query-state-record.h File be/src/service/query-state-record.h: http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/query-state-record.h@318 PS28, Line 318: const std::shared_ptr<QueryStateRecord> base_state_src = nullptr); Why make this parameter optional? The code in QueryStateRecord assumes base_state_src is not nullptr. http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/query-state-record.cc File be/src/service/query-state-record.cc: http://gerrit.cloudera.org:8080/#/c/20762/28/be/src/service/query-state-record.cc@204 PS28, Line 204: base_state->num_rows_fetched = exec_state.num_rows_fetched_counter(); Fixed this in my patch. Good catch! http://gerrit.cloudera.org:8080/#/c/20762/28/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/28/fe/src/main/java/org/apache/impala/catalog/SystemTable.java@51 PS28, Line 51: public static final String QUERY_LIVE = "impala_query_live"; Thoughts on making the query live table name a hidden configuration option? It's not nearly as important as having the completed queries table configurable because there are far more problems likely to happen with the completed queries table than the live queries table. http://gerrit.cloudera.org:8080/#/c/20762/28/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/28/fe/src/main/java/org/apache/impala/planner/SystemTableScanNode.java@59 PS28, Line 59: checkForSupportedFileFormats(); Can this function call be skipped? We aren't using any complex types. Or is the concern that in the future there may be new types of system tables that need to be prevented from using complex types? http://gerrit.cloudera.org:8080/#/c/20762/28/tests/custom_cluster/test_query_live.py File tests/custom_cluster/test_query_live.py: http://gerrit.cloudera.org:8080/#/c/20762/28/tests/custom_cluster/test_query_live.py@34 PS28, Line 34: def validate_columns(self, data, result, completed=True): We should be able to do a little bit of work to the new test/util/workload_management.py file to eliminate this function. http://gerrit.cloudera.org:8080/#/c/20762/28/tests/custom_cluster/test_query_live.py@370 PS28, Line 370: def test_query_live(self, vector): Please consider adding asserts on the query plan and exec summary in the profile for the new system table and scan node http://gerrit.cloudera.org:8080/#/c/20762/28/tests/custom_cluster/test_query_live.py@409 PS28, Line 409: itwith Nit: need space -- 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: 28 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-Comment-Date: Fri, 08 Mar 2024 23:03:47 +0000 Gerrit-HasComments: Yes
