impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
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:

(20 comments)

Thanks for the review, please see PS27.

http://gerrit.cloudera.org:8080/#/c/2200/24/be/src/scheduling/simple-scheduler.h
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
Done.


http://gerrit.cloudera.org:8080/#/c/2200/25/be/src/scheduling/simple-scheduler.h
File be/src/scheduling/simple-scheduler.h:

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


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


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


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
into
> 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
of
> "lowest number"
Done


Line 174:   /// update their key. For each plan node we create a new heap, so they are not
shared
> 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?
Done


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


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


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


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


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


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


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


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
Done


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


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 http://gerrit.cloudera.org:8080/2200
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I044f83806fcde820fcb38047cf6b8e780d803858
Gerrit-PatchSet: 25
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message