Kathy Sun has posted comments on this change. Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page ......................................................................
Patch Set 7: (13 comments) http://gerrit.cloudera.org:8080/#/c/3998/6/be/src/util/metrics-test.cc File be/src/util/metrics-test.cc: PS6, Line 350: reinterpret_cast<MetricGroup*>(NULL) > just wondering - does nullptr work here? Yeah, It could work, I write in this way since lines 372 did so. to make them consistent http://gerrit.cloudera.org:8080/#/c/3998/6/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: PS6, Line 27: quest_w > should be: Done PS6, Line 37: "Test all memz/ webpages""" > if you raise here, line 38 will never be executed. I think you don't need t Done PS6, Line 27: def request_web_page(\ : host_name, web_ui_port, relative_url, params={}, timeout_secs=DEFAULT_TIMEOUT): : url = "http://%s:%s%s"; % (host_name, web_ui_port, relative_url) : resp = requests.get(url, params=params, timeout=timeout_secs) : resp.raise_for_status() : return resp : : class TestWebPage(ImpalaTestSuite): : """request web page /memz at imapalad / statestored / catalogd.""" : def test_memz(self): : """Test all memz/ webpages""" : relative_url = "/memz" : > Don't think you need a class for this. Just have: Done PS6, Line 44: ge = request_web_page("lo > comment needs finishing Done PS6, Line 45: assert page.status_cod > I feel like this test can be written like: Done PS6, Line 49: > Does this fail if the web page doesn't exist? I add assert for the status_code, so it will fail now http://gerrit.cloudera.org:8080/#/c/3998/7/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: PS7, Line 17: # : # Verification of impalad metrics after a test run. > remove Done PS7, Line 21: from tests.common.errors import Timeout > is this used? no, removed PS7, Line 21: from tests.common.errors import Timeout > is this used? Done PS7, Line 27: (\ > remove the \, and try and put some of the parameters onto this line. I decided not to wrap requests.get() into another function remove request_web_page() for cleaner code PS7, Line 31: resp.raise_for_status() > does this raise an error when status != 200? If so you don't need the check I think status_code is enough for our purpose of checking whether the system crashed on clicking /memz. PS7, Line 35: """request web page /memz at imapalad / statestored / catalogd.""" > I'd remove this, it doesn't tell the reader anything that's not in the test Done -- To view, visit http://gerrit.cloudera.org:8080/3998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Kathy Sun <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Kathy Sun <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
