Vuk Ercegovac 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: (11 comments) main change with the patch is to separate internal vs. client init/start. 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. yup, I had already updated the front end to not return anything. I simplified the SetCatalogInitialized method in the same manner. 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 loc removed and simplified the setcataloginitialized method in the same manner. 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 h yes, was able to repro a case where I restart an impalad (but disallow its catalog to be initialized), I submit a query to another impalad whose catalog is initialized, a query fragment gets sent to the restarted impalad, its connection is refused, so the query dies. I've changed things so that internal vs. client init and start are separated. Now, an impalad can process work even though if its supposed to come up as a coordinator (and is still waiting on the catalog). http://gerrit.cloudera.org:8080/#/c/8202/12/be/src/service/impala-server.cc@2045 PS12, Line 2045: if (thrift_be_server_.get()) { > Yeah, we should make sure we have test cases for this and the various coord working on them... starting/stopping cluster processes does not interact well with the e2e tests. I was able to manually start up a cluster that works for the repro above. 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 th Done 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()? Done 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 wha Done 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 Done 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 misinterpre Done 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 Done 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 cas I was a bit surprised to see authorization configs here-- what was the intent with regard to catalog readiness in the first place? If the authorization tests do not matter, then I'll remove the test. -- 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: Wed, 25 Oct 2017 17:25:22 +0000 Gerrit-HasComments: Yes
