impala-reviews mailing list archives

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

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