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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 31 Oct 2017 17:30:36 +0000 Gerrit-HasComments: Yes
