impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
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. ( )

Change subject: IMPALA-4704: Turns on client connections when local catalog initialized.

Patch Set 12:

File be/src/service/frontend.h:
PS12, Line 171:   Status WaitForCatalog();
Why even return anything? Success is implied by this function returning.
File be/src/service/
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.
File be/src/service/
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.
File bin/
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
PS12, Line 255: def is_catalog_ready(impala_cluster):

To distinguish the other "catalog ready" which refers to the local catalog cache of an impalad.
File fe/src/main/java/org/apache/impala/service/
PS12, Line 865:"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.
PS12, Line 866:     int num_tries = 0;
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."
File fe/src/main/java/org/apache/impala/service/
PS12, Line 583:   public void setCatalogInitialized() {
setCatalogIsReady() for consistency
File fe/src/test/java/org/apache/impala/service/
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Balazs Jeszenszky <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:03:15 +0000
Gerrit-HasComments: Yes

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