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 Tue, 07 Nov 2017 06:14:54 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 19:

(5 comments)

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

http://gerrit.cloudera.org:8080/#/c/8202/19/be/src/service/frontend.h@162
PS19, Line 162: Sets the FE catalog to be initialized.
> I don't understand what that's trying to say.
updated the comment to better reflect the method renaming.


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

http://gerrit.cloudera.org:8080/#/c/8202/19/be/src/service/impala-server.h@79
PS19, Line 79: The executor role starts ImpalaInternalService API's
> technically the coordinator also has to start the ImpalaInternalService API
Done


http://gerrit.cloudera.org:8080/#/c/8202/19/be/src/service/impala-server.h@89
PS19, Line 89: must call the following
             : /// methods:
> why is this split into two steps? Is there something the caller would want 
good question-- I've kept it the same as before so not sure what the reason was to keep them
separated. currently, there is a check in impalad-main in between init and start, but perhaps
we can do without that.


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

http://gerrit.cloudera.org:8080/#/c/8202/19/be/src/service/impala-server.cc@1518
PS19, Line 1518:    
> nit: indentation glitch
Done


http://gerrit.cloudera.org:8080/#/c/8202/19/be/src/service/impala-server.cc@1937
PS19, Line 1937:   RETURN_IF_ERROR(exec_env_->StartServices());
> why does this have to be moved so early? i'm not sure if this will cause pr
I need it for the statestore subscriber to receive the catalog. L1939 will not work otherwise.
Not sure why tests did not run into any issues. For now, I've separated them so that krpc,
when enabled, starts at the same time as before this change.



-- 
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: 19
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: Juan Yu <jyu@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, 07 Nov 2017 06:14:54 +0000
Gerrit-HasComments: Yes

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