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 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/8202/12/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/8202/12/be/src/service/frontend.h@171 PS12, Line 171: Status WaitForCatalog(); Why even return anything? Success is implied by this function returning. http://gerrit.cloudera.org:8080/#/c/8202/12/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/8202/12/be/src/service/frontend.cc@253 PS12, Line 253: JniLocalFrame jni_frame; This JNI frame stuff is not needed here because we are not creating any local references. The same is true for SetCatalogInitialized() - frame stuff not needed. http://gerrit.cloudera.org:8080/#/c/8202/12/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8202/12/be/src/service/impala-server.cc@2045 PS12, Line 2045: if (thrift_be_server_.get()) { I wonder if this can cause problems. A coordinator+executor impalad might have registered with the statestore but not received the initial catalog update yet. This will not stop other coordinators from scheduling work on this impalad - but the InternalService port has not yet been opened, meaning those queries will fail. I think we need to strictly distinguish between the internal and client facing services. http://gerrit.cloudera.org:8080/#/c/8202/12/bin/start-impala-cluster.py File bin/start-impala-cluster.py: http://gerrit.cloudera.org:8080/#/c/8202/12/bin/start-impala-cluster.py@81 PS12, Line 81: parser.add_option("--disable_catalog", dest="disable_catalog", default=False, disable_catalogd? We seem to be consistently referring to processes with their "d" suffix http://gerrit.cloudera.org:8080/#/c/8202/12/bin/start-impala-cluster.py@255 PS12, Line 255: def is_catalog_ready(impala_cluster): is_catalogd_up()? To distinguish the other "catalog ready" which refers to the local catalog cache of an impalad. http://gerrit.cloudera.org:8080/#/c/8202/12/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/12/fe/src/main/java/org/apache/impala/service/Frontend.java@865 PS12, Line 865: LOG.info("Waiting for local catalog to be initialized."); Users may not know what "initialized" means, better to state explicitly what we are waiting for, e.g.: Waiting for the first catalog update from the statestore. http://gerrit.cloudera.org:8080/#/c/8202/12/fe/src/main/java/org/apache/impala/service/Frontend.java@866 PS12, Line 866: int num_tries = 0; numTries http://gerrit.cloudera.org:8080/#/c/8202/12/fe/src/main/java/org/apache/impala/service/Frontend.java@905 PS12, Line 905: "Analyzing a query is not support when the local catalog is not initialized."); I'd chose a different phrasing because "not supported" could be misinterpreted as a missing feature or might make the user think he/she did something wrong - but hitting this is not expected and probably a bug on our side. How about: "Local catalog has not been initialized. Aborting query analysis." http://gerrit.cloudera.org:8080/#/c/8202/12/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/8202/12/fe/src/main/java/org/apache/impala/service/JniFrontend.java@583 PS12, Line 583: public void setCatalogInitialized() { setCatalogIsReady() for consistency 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: Can you think of a reason for keeping this test around? The interesting case was the "not ready" case which is now gone, so I'm not sure this test makes 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: 12 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, 24 Oct 2017 00:03:15 +0000 Gerrit-HasComments: Yes
