impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4704: Turns on client connections when local catalog initialized.
Date Tue, 31 Oct 2017 05:11:18 GMT
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8202 )

Change subject: IMPALA-4704: Turns on client connections when local catalog initialized.
......................................................................


Patch Set 16:

(9 comments)

Nice, this looks much better

http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.h@105
PS16, Line 105: /// TODO: The state of a running query is currently not cleaned up if the
Let's document the startup procedure and the reasoning behind it here


http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.h@1014
PS16, Line 1014:   /// Flag that records if backend and/or client services have been started.
Clarify the meaning of "and/or" here. The distinction seems important. Will this flag be set
if only one service has been started or not?


I know I suggested this new flag :), but now I'm wondering whether we can use the existing
"is_ready" metric for this purpose.


http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.h@1017
PS16, Line 1017:   boost::mutex services_started_lock_;
std::atomic_bool instead of this lock?


http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.cc@1626
PS16, Line 1626:   // Register this backend only if services have been started.
Feels clearer to me to check this at the caller. Can be hard to reason about functions that
are no-ops depending on internal state.
Any reason to have the check in here?

In any case we should add a function comment for the new behavior (ideally in the membership
callback assuming you agree to move this check)


http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.cc@1933
PS16, Line 1933: Status ImpalaServer::Init(int32_t thrift_be_port, int32_t beeswax_port,
Why reformat fn args? Seemed ok the way it was.


http://gerrit.cloudera.org:8080/#/c/8202/16/tests/custom_cluster/test_catalog_wait.py
File tests/custom_cluster/test_catalog_wait.py:

http://gerrit.cloudera.org:8080/#/c/8202/16/tests/custom_cluster/test_catalog_wait.py@24
PS16, Line 24:   """Impalad must wait for the catalog prior to opening up client ports.
Specify the waiting condition more precisely. An impalad must wait for the initial catalog
update to arrive via the statestore.


http://gerrit.cloudera.org:8080/#/c/8202/16/tests/custom_cluster/test_catalog_wait.py@54
PS16, Line 54:     self.expect_connection(self.cluster.impalads[0])
Ideally we'd also check the internal service


http://gerrit.cloudera.org:8080/#/c/8202/16/tests/custom_cluster/test_catalog_wait.py@59
PS16, Line 59:   def test_query_subset(self):
Can we combine this test wit the one above? They seem very similar


http://gerrit.cloudera.org:8080/#/c/8202/16/tests/custom_cluster/test_catalog_wait.py@71
PS16, Line 71:     self.cluster.impalads[0].service.get_metric_value('impala-server.ready',
1);
I'm wondering whether this can be flaky. We often use self.wait_for_metric_value() in case
we expect delays. These impalads might still be waiting for the initial catalog update.



-- 
To view, visit http://gerrit.cloudera.org:8080/8202
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52b881cba18a7e4533e21a78751c2e35c3d4c8a6
Gerrit-Change-Number: 8202
Gerrit-PatchSet: 16
Gerrit-Owner: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <jeszyb@gmail.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Oct 2017 05:11:18 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message