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, 31 Oct 2017 17:30:36 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 17:

(7 comments)

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

http://gerrit.cloudera.org:8080/#/c/8202/17/be/src/service/impala-server.h@81
PS17, Line 81: /// 
> whitespace
Done


http://gerrit.cloudera.org:8080/#/c/8202/17/be/src/service/impala-server.h@85
PS17, Line 85: /// by clients at the same time.
> Might want to weave in something like this to motivate the specific startup
Done


http://gerrit.cloudera.org:8080/#/c/8202/17/be/src/service/impala-server.h@87
PS17, Line 87: /// 1. Init
> Init()
Done


http://gerrit.cloudera.org:8080/#/c/8202/17/be/src/service/impala-server.h@89
PS17, Line 89: ///    - Wait (indefinitelt) for local catalog to be initialized from statestore
(if coordinator)
> typo: indefinitely
Done


http://gerrit.cloudera.org:8080/#/c/8202/17/be/src/service/impala-server.h@93
PS17, Line 93: /// 2. Start
> Start()
Done


http://gerrit.cloudera.org:8080/#/c/8202/17/be/src/service/impala-server.h@98
PS17, Line 98: /// Membership callback thread:
> When is the 'is_ready' metric set?
currently, the main flips this bit-- its the one orchestrating the sequence so it knows when
its ready. since we've now equated services_started_ and the ready flag, I'll consolidate
them and move them to the server. perhaps there were other things the main wanted to do before
announcing itself as ready, but for now, they're 1:1.


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

http://gerrit.cloudera.org:8080/#/c/8202/16/be/src/service/impala-server.h@1014
PS16, Line 1014:   ExpirationQueue queries_by_timestamp_;
> Fair point.
I think the thing I'm objecting to here is that the internal state will depend on an external
view. I'd prefer the arrows to be oriented only from internal to external (external depends
on internal), even at the expense of storing this value twice. By using the metric, its the
same as accessing a global. I don't see any example where we use metric values for internal
state/logic-- afaict, they're used for sanity checks or logging (in addition to their intended
purpose). I think its preferable to keep it that way.



-- 
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: 17
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, 31 Oct 2017 17:30:36 +0000
Gerrit-HasComments: Yes

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