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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 31 Oct 2017 05:11:18 +0000 Gerrit-HasComments: Yes
