impala-reviews mailing list archives

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

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

Patch Set 12:


main change with the patch is to separate internal vs. client init/start.
File be/src/service/frontend.h:
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.
File be/src/service/
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.
File be/src/service/
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).
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.
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 th
PS12, Line 255: def is_catalog_ready(impala_cluster):
> is_catalogd_up()?
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 wha
PS12, Line 866:     int num_tries = 0;
> numTries
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
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 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
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: Wed, 25 Oct 2017 17:25:22 +0000
Gerrit-HasComments: Yes

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