impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page
Date Fri, 19 Aug 2016 06:08:02 GMT
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3981: Fix crash when accessing statestored / catalogd /memz page

Patch Set 6:


Glad to see the test!
File be/src/util/

PS6, Line 349:   
single space only

PS6, Line 350: reinterpret_cast<MetricGroup*>(NULL)
just wondering - does nullptr work here?
File tests/webserver/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If59b10f20044d1a468f27810a3029fe18fb19f29
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Kathy Sun <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Kathy Sun <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message