impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4704: Turns on client connections when local catalog initialized.
Date Wed, 25 Oct 2017 17:25:22 GMT
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 <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: Wed, 25 Oct 2017 17:25:22 +0000
Gerrit-HasComments: Yes

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