impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
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:

(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 <kathy.sun@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Kathy Sun <kathy.sun@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message