impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5147: Add the ability to exclude hosts from query execution
Date Tue, 18 Apr 2017 00:14:52 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5147: Add the ability to exclude hosts from query execution
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6628/1/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

Line 207:   }
> Essentially the scheduler only manages/uses backends that are executors. Ho
What else would we want to rename backend->executor besides backend_config_ and associated
functions/variables?  (The types themselves don't need to be updated in this change, but it's
helpful to distinguish what backends the sets themselves include). That doesn't seem too bad.
 Or are there others?

Given that we now have coord_only_backend_config_, it seems that renaming would be helpful
to clarify what backend_config_ actually contains.


http://gerrit.cloudera.org:8080/#/c/6628/4/be/src/scheduling/scheduler.cc
File be/src/scheduling/scheduler.cc:

Line 202:     // If this is not an executor, don't add it to the new backend config.
this comment just repeats what the code is doing, but doesn't explain why. it would be more
useful if at the top of the function we just say at a high level what this routine does:

// Update executors_config_ and current_executors_ to match the set of executors given by
the subscriber_topic_updates.

Or something like that.


http://gerrit.cloudera.org:8080/#/c/6628/4/be/src/scheduling/scheduler.h
File be/src/scheduling/scheduler.h:

PS4, Line 269: backend_config_
as mentioned earlier, this would be easier to follow if this were renamed to signify that
it only contains executors.


PS4, Line 283:   /// Map from unique backend id to TBackendDescriptor. Used to track the known
executors
             :   /// from the statestore. It's important to track both the backend ID as well
as the
             :   /// TBackendDescriptor so we know what is being removed in a given update.
this comment was very unhelpful at understanding the code because it doesn't explicitly say
what a "backend id" is, or what this map really is.  After reading the code you can see it's
just a full representation of the IMPALA_MEMBERSHIP_TOPIC {key, value} pairs for executors
(to which deltas can be applied, and it drives updates to state, e.g. backend_config_, that's
derived from the topic). It would be helpful to be explicit about that.


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

PS4, Line 41: only_coordinate
when i read this name I thought that it would start a cluster of only coordinators (which
was confusing).  how about --exclusive_coordinators? Though that name isn't great either.


-- 
To view, visit http://gerrit.cloudera.org:8080/6628
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5d2ff7f341c9d2b0649e4d14561077e166ad7c4d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message