Henry Robinson has posted comments on this change. Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page ......................................................................
Patch Set 6: (8 comments) Glad to see the test! http://gerrit.cloudera.org:8080/#/c/3998/6/be/src/util/metrics-test.cc File be/src/util/metrics-test.cc: PS6, Line 349: single space only PS6, Line 350: reinterpret_cast<MetricGroup*>(NULL) just wondering - does nullptr work here? http://gerrit.cloudera.org:8080/#/c/3998/6/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: PS6, Line 27: webinfo should be: class Webinfo(object): - but I think we should remove the class. PS6, Line 37: raise Timeout(underlying_exception=e) if you raise here, line 38 will never be executed. I think you don't need the try / except blocks, unless there's some specific error condition you want to deal with here. PS6, Line 27: class webinfo: : def __init__(self): : self.host_name = "localhost" : self.web_ui_port = None : : def _request_web_page(self, relative_url, params={}, timeout_secs=DEFAULT_TIMEOUT): : url = "http://%s:%s%s"; % (self.host_name, self.web_ui_port, relative_url) : try: : resp = requests.get(url, params=params, timeout=timeout_secs) : except requests.exceptions.Timeout as e: : raise Timeout(underlying_exception=e) : resp.raise_for_status() : return resp Don't think you need a class for this. Just have: def request_web_page(...): you can pass the port in yourself. PS6, Line 44: "Test that all the memz/" comment needs finishing PS6, Line 45: relative_url = "/memz" I feel like this test can be written like: for host, port in (("localhost", 25000), ("localhost", 25010), ("localhost", 25020): page = request_web_page(host, port, "/memz") # assert whatever you need to about 'page' here PS6, Line 49: impalad._request_web_page(relative_url) Does this fail if the web page doesn't exist? -- 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: 6 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
