Daniel Becker 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 4: (12 comments) Thanks Riza! http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@9 PS4, Line 9: build Nit: builds. http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@14 PS4, Line 14: relax Nit: relaxes. http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@14 PS4, Line 14: simplify Nit: simplifies. http://gerrit.cloudera.org:8080/#/c/21100/4//COMMIT_MSG@15 PS4, Line 15: add Nit: adds. http://gerrit.cloudera.org:8080/#/c/21100/4/be/src/util/impalad-metrics.h File be/src/util/impalad-metrics.h: http://gerrit.cloudera.org:8080/#/c/21100/4/be/src/util/impalad-metrics.h@264 PS4, Line 264: that Nit: that are currently. http://gerrit.cloudera.org:8080/#/c/21100/4/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/21100/4/common/thrift/metrics.json@2857 PS4, Line 2857: that currently Nit: that are currently. http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py File tests/custom_cluster/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@173 PS4, Line 173: log_metric = metric Could add a break here. Or do we reach this multiple times and we need the last one? http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@174 PS4, Line 174: response.text Before writing the response text we could add some info about what's coming, for example "Metrics: " or "Metrics received from WebUI: ". http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178 PS4, Line 178: test Nit: the test queries. http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178 PS4, Line 178: query Nit: the query page. http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@178 PS4, Line 178: subset Nit: a subset. http://gerrit.cloudera.org:8080/#/c/21100/4/tests/custom_cluster/test_web_pages.py@180 PS4, Line 180: json.loads(response.text) The text is already parsed into JSON on L166, we could move this variable there so we don't have to parse the text twice. -- 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: 4 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: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Mon, 04 Mar 2024 15:02:43 +0000 Gerrit-HasComments: Yes
