Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/21100 )
Change subject: IMPALA-12864: Deflake test_query_log_size_in_bytes. ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG@14 PS5, Line 14: This patch simplifies the test query add relaxes the assertion. This Nit: and relaxes the assertion http://gerrit.cloudera.org:8080/#/c/21100/5//COMMIT_MSG@15 PS5, Line 15: patch also adds QUERY_LOG_EST_TOTAL_SIZE metric that is validated as Please mention the name of the new "impala-server.query-log-est-total-size" metric as well. http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/impala-server.h@1137 PS5, Line 1137: /// QUERY_LOG_EST_TOTAL_SIZE metric. The lock does not protect the QUERY_LOG_EST_TOTAL_SIZE when it's being read by the Impala http server. I don't think that's an issue since the metric is initialized once, and that initialization happens long before the Impala http server can access the metric. As far as I know, the only potential issue is the impala-server.query-log-est-total-size metric may be slightly mis-reported, but that is ok since the Impala http server always lags behind the query_log_ anyways. http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/query-state-record.h File be/src/service/query-state-record.h: http://gerrit.cloudera.org:8080/#/c/21100/5/be/src/service/query-state-record.h@181 PS5, Line 181: int64_t peak_memory_usage = 0; Thanks for fixing! http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py File tests/custom_cluster/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py@153 PS5, Line 153: impalad_args="--query_log_size_in_bytes=" + str(40 * 1024) Nit: Since the (40 * 1024) calculation is used in two places, it would eliminate the repetition to declare a variable above this test function as use that variable in both places. http://gerrit.cloudera.org:8080/#/c/21100/5/tests/custom_cluster/test_web_pages.py@178 PS5, Line 178: assert log_metric["value"] <= 40 * 1024, log_metric The above code can be replaced with: impalad = self.cluster.get_first_impalad() assert 0 < impalad.service.get_metric_value("impala-server.query-log-est-total-size") < 40 * 1024 -- To view, visit http://gerrit.cloudera.org:8080/21100 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic9b543ffa931a01c11d58e45774ea46af78b3a25 Gerrit-Change-Number: 21100 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@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 19:43:04 +0000 Gerrit-HasComments: Yes
