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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
