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 Mon, 30 Oct 2017 21:36:52 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 15:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/frontend.cc
File be/src/service/frontend.cc:

http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/frontend.cc@31
PS15, Line 31: #ifndef NDEBUG
What's the harm in always compiling it in?


http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/frontend.cc@256
PS15, Line 256:   if (result) return true;
return result?


http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.h@125
PS15, Line 125:   /// Initializes client RPC services. Must be called after InitInternalServices.
Is a
we typically say InitInternalServices() to make it clear it's a function


http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impala-server.cc@1541
PS15, Line 1541:     // Additionally wait for the local catalog to be initialized if the server
is a
Suggest rephrasing "wait" to clarify. "wait" could be misinterpreted as blocking. Maybe say
something like:

Announce the availability of this impalad coordinator once the local catalog has been initialized.


http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

http://gerrit.cloudera.org:8080/#/c/8202/15/be/src/service/impalad-main.cc@91
PS15, Line 91:   Status status = impala_server->StartInternalServices();
As discussed, ordering is not quite right yet.

It might make sense to add flags for client_services_started and internal_services_started
to address the race between opening internal/client ports and announcing the availability
of this daemon through the membership callback.


http://gerrit.cloudera.org:8080/#/c/8202/15/bin/start-impala-cluster.py
File bin/start-impala-cluster.py:

http://gerrit.cloudera.org:8080/#/c/8202/15/bin/start-impala-cluster.py@81
PS15, Line 81: # For testing: list of comma-separated delays, in milliseconds, that delay
catalog
clarify which catalog exactly


http://gerrit.cloudera.org:8080/#/c/8202/15/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/15/fe/src/main/java/org/apache/impala/service/Frontend.java@877
PS15, Line 877:     }
add a final LOG.info() here declaring success (and remove the one in impala-server)


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: 
> I was a bit surprised to see authorization configs here-- what was the inte
Sentry can configured in a service mode or with a file-based auth policy. I suppose this test
was checking that the readiness check works regardless of auth policy. Not sure how much sense
that makes.

For auth we use AuthorizationTest.

I'm thinking this test doesn't make 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: 15
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: Mon, 30 Oct 2017 21:36:52 +0000
Gerrit-HasComments: Yes

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