Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20770 )
Change subject: IMPALA-12426: Query History Table ...................................................................... Patch Set 31: (16 comments) http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/internal-server.cc File be/src/service/internal-server.cc: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/service/internal-server.cc@117 PS13, Line 117: !UUIDEmpty(internal_query_id)) { > Use the new function in uid-util.h Done http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/12/be/src/service/workload-management.cc@646 PS12, Line 646: fields_.push_back(MakeTuple("exec_summary", _exec_summary)); : fields_.push_back(MakeTuple("plan", _plan)); : fields_.push_back(MakeTuple("num_rows_fetched", _num_rows_fetched, "BIGINT")); : fields_.push_back(MakeTuple( : "row_materialization_bytes_per_sec", _row_materialization_rate, "BIGINT")); : fields_.push_back(MakeTuple( : "row_materializ > I'm going to rearrange these into a different order that puts the string ch Done http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc File be/src/service/workload-management.cc: http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@171 PS31, Line 171: SHUTDOWN_REQUESTED nit: SHUTTING_DOWN http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@769 PS31, Line 769: completed_queries_thread_state_ == RUNNING Can ShutdownWorkloadManagement called before completed_queries_thread_state_ becomes RUNNING? http://gerrit.cloudera.org:8080/#/c/20770/31/be/src/service/workload-management.cc@841 PS31, Line 841: completed_queries_thread_state_ What if the previous completed_queries_thread_state_ is SHUTDOWN_REQUESTED or SHUTDOWN here? http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/network-util.h File be/src/util/network-util.h: http://gerrit.cloudera.org:8080/#/c/20770/13/be/src/util/network-util.h@114 PS13, Line 114: turn lhs.hostname() == rhs.hostname() && lhs.port() == rhs.port() > Should uds_address be the last tie breaker? Done http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py File tests/beeswax/impala_beeswax.py: http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py@185 PS31, Line 185: fetch_profile_after_close=False fetch_profile_after_close=True seems specific to TestQueryLogTable tests only. Can this variable turned into a Client field member and set during Client initialization? Tests that require this behavior can then specify it during create_connection(). http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py@198 PS31, Line 198: runtime_profile = self.get_runtime_profile(handle) Skip pulling profile if !fetch_profile_after_close http://gerrit.cloudera.org:8080/#/c/20770/31/tests/beeswax/impala_beeswax.py@219 PS31, Line 219: result.runtime_profile = self.get_runtime_profile(handle) Skip pulling profile if !fetch_profile_after_close http://gerrit.cloudera.org:8080/#/c/20770/31/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/20770/31/tests/common/custom_cluster_test_suite.py@244 PS31, Line 244: method=None this is unused? http://gerrit.cloudera.org:8080/#/c/20770/9/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/20770/9/tests/custom_cluster/test_query_log.py@359 PS9, Line 359: se: : assert exec_group is not None : assert data[index] == exec_group.group(1), "executor > I didn't know it was possible to get the profile from the query handle. I Done http://gerrit.cloudera.org:8080/#/c/20770/12/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/20770/12/tests/custom_cluster/test_query_log.py@633 PS12, Line 633: catalogd_args="--enable_workload_mgmt", : impalad_graceful_shutdown=True) : @pytest.mark.parametrize("client_protocol", PROTOCOL_ALL) : def test_query_log_table_ddl(self, client_protocol): : """Asserts the values written to the query log table match the values from the : query > Maybe it is due to stale metadata. Is it worth to run "invalidate metadata Moved discussion to patch set 31. http://gerrit.cloudera.org:8080/#/c/20770/31/tests/custom_cluster/test_query_log.py File tests/custom_cluster/test_query_log.py: http://gerrit.cloudera.org:8080/#/c/20770/31/tests/custom_cluster/test_query_log.py@421 PS31, Line 421: assert_byte_str(expected_str=row_mat_str, actual_bytes=data[index], : msg="row materialization rate incorrect", factor=1000) : nit: maybe it is worth to create separate function like "assert_byte_rate_str" that hide the default factor=1000. It does not immediately clear to me what factor is in relation with "byte_str". http://gerrit.cloudera.org:8080/#/c/20770/31/tests/custom_cluster/test_query_log.py@1221 PS31, Line 1221: # Allow time for Impala to process the insert. : sleep(3) I think this sleep can be replaced with execution of REFRESH query. client.execute("refresh " + self.QUERY_TBL) http://gerrit.cloudera.org:8080/#/c/20770/31/tests/util/assert_time.py File tests/util/assert_time.py: http://gerrit.cloudera.org:8080/#/c/20770/31/tests/util/assert_time.py@27 PS31, Line 27: "{0} -- expected: {1}, actual {2}, calculated {3}" \ : .format(msg, expected_str, actual_time_ns, total_nanoseconds) Print the tolerance value in assert message (total_nanoseconds * tolerance). http://gerrit.cloudera.org:8080/#/c/20770/31/tests/util/memory.py File tests/util/memory.py: http://gerrit.cloudera.org:8080/#/c/20770/31/tests/util/memory.py@27 PS31, Line 27: "{0} -- expected: {1}, " \ : "actual: {2}, calculated: {3}".format(msg, expected_str, actual_bytes, calc) Print the tolerance value in assert message (calc * 0.005). -- To view, visit http://gerrit.cloudera.org:8080/20770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2d2da9d450fba4e789400cfa62927fc25d34f844 Gerrit-Change-Number: 20770 Gerrit-PatchSet: 31 Gerrit-Owner: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Mon, 04 Mar 2024 23:42:37 +0000 Gerrit-HasComments: Yes
