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 15: (8 comments) http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/frontend.cc@31 PS15, Line 31: #ifndef NDEBUG What's the harm in always compiling it in? http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/frontend.cc@256 PS15, Line 256: if (result) return true; return result? http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.h@125 PS15, Line 125: /// Initializes client RPC services. Must be called after InitInternalServices. Is a we typically say InitInternalServices() to make it clear it's a function http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.cc@1541 PS15, Line 1541: // Additionally wait for the local catalog to be initialized if the server is a Suggest rephrasing "wait" to clarify. "wait" could be misinterpreted as blocking. Maybe say something like: Announce the availability of this impalad coordinator once the local catalog has been initialized. http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impalad-main.cc@91 PS15, Line 91: Status status = impala_server->StartInternalServices(); As discussed, ordering is not quite right yet. It might make sense to add flags for client_services_started and internal_services_started to address the race between opening internal/client ports and announcing the availability of this daemon through the membership callback. http://gerrit.cloudera.org:8080/#/c/8202/15/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/8202/15/bin/start-impala-cluster.py@81 PS15, Line 81: # For testing: list of comma-separated delays, in milliseconds, that delay catalog clarify which catalog exactly http://gerrit.cloudera.org:8080/#/c/8202/15/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/8202/15/fe/src/main/java/org/apache/impala/service/Frontend.java@877 PS15, Line 877: } add a final LOG.info() here declaring success (and remove the one in impala-server) http://gerrit.cloudera.org:8080/#/c/8202/12/fe/src/test/java/org/apache/impala/service/FrontendTest.java File fe/src/test/java/org/apache/impala/service/FrontendTest.java: http://gerrit.cloudera.org:8080/#/c/8202/12/fe/src/test/java/org/apache/impala/service/FrontendTest.java@a69 PS12, Line 69: > I was a bit surprised to see authorization configs here-- what was the inte Sentry can configured in a service mode or with a file-based auth policy. I suppose this test was checking that the readiness check works regardless of auth policy. Not sure how much sense that makes. For auth we use AuthorizationTest. I'm thinking this test doesn't make sense anymore. -- 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: 15 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: Mon, 30 Oct 2017 21:36:52 +0000 Gerrit-HasComments: Yes
