impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-2979: Fix scheduling on remote hosts
Date Sun, 17 Jul 2016 14:12:56 GMT
Lars Volker has posted comments on this change.

Change subject: IMPALA-2979: Fix scheduling on remote hosts

Patch Set 25:


Thanks for the review, please see PS27.
File be/src/scheduling/simple-scheduler.h:

Line 380:       const TResourceBrokerReservationResponse& reservation, Coordinator* coord);
> the comments needs to describe the externally visible behavior, which inclu
File be/src/scheduling/simple-scheduler.h:

Line 91:   /// List of backends.
> no need to repeat/paraphrase type name

Line 112:     /// Construct maps from list of backends.
> config

Line 137:   /// Internal structure to track scan range assignments for each backend and prioritize
> "for a backend". remove comment about prioritization, this structure doesn'

Line 141:     /// The number of bytes assigned to a backend host. Part of the key of
> backend or backend host?
Backend host. We track assigned bytes per IP address, as we only support a single backend
per host.

Line 142:     /// AddressableAssignmentHeap. This gets initialized when elements are inserted
> don't comment on other data structures here
I had added this comment in response to the previous remark ("add comments for fields (who
sets it, invariants)"). As this is a struct it is directly modified by in AddressableAssignmentHeap,
it doesn't have member functions. Can you make a suggestion for a comment?

Line 164:   /// will make the top() element of the heap be the backend with the least number
> "lowest number"

Line 174:   /// update their key. For each plan node we create a new heap, so they are not
> the last sentence should go into the structure that contains an object of t
I had added this to address this review comment: "where is it used, who creates it, is this
shared between threads?" The containing structure (AssignmentCtx) already contains a similar
comment. Can you make a suggestion what to put here?

Line 199:   /// member in AssignmentCtx.
> move into AssignmentCtx? is it used outside of that?

Line 222:     /// data_locations:     List of candidate replica IP addresses, from which the
> redundant w/ earlier part of comment

Line 224:     /// break_ties_by_rank: Whether to consider the random rank during backend selection.
> same here

Line 229:     /// those will be preferred. Otherwise one is selected from the assignment heap.
> describe the externally observable behavior (what are the characteristics o

Line 233:     /// pointer. This assumes that a returned backend will also be assigned to.
The caller
> "increment the internal pointer" isn't meaningful

Line 242:         const TScanRangeLocations& scan_range_locations,
> role of this param?

Line 252:     /// Used to lookup hostnames to IP addresses and IP addresses to backend.
> use verb, not noun

Line 291:   /// The scheduler's backend maps. When receiving changes to the backend configuration
> replace use of term 'backend maps' throughout

Line 292:   /// from the statestore we will make a copy of those maps, apply the updates to
the copy
> this is fairly central to how this class functions, and should be pointed o
Done. Keep here as well or remove?

Line 424:   /// assigned work is picked, thus aiming to distribute the work as even as possible.
> as evenly

Line 426:   /// Finally scan ranges are considered, which do not have an impalad backend running
> remove comma

Line 446:   ///   amount of assigned work), then the first one will be picked deterministically.
> only true for disk replicas, no?
Yes, done

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I044f83806fcde820fcb38047cf6b8e780d803858
Gerrit-PatchSet: 25
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Reviewer: anujphadke <>
Gerrit-HasComments: Yes

View raw message