Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2979: Fix scheduling on remote hosts ......................................................................
Patch Set 22: (28 comments) some of these comments were written prior to our conversation at 5pm today. ignore the ones that don't make sense anymore. http://gerrit.cloudera.org:8080/#/c/2200/22/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 236: for(const BackendList::value_type& backend: backends) { for ( Line 271: for(const TTopicItem& item: delta.topic_entries) { formatting (and elsewhere) Line 312: // TODO: Currently each non-empty statestore update triggers a rebuild of these collect bigger todos at the bottom of the class comment in the .h that way you have a reasonably complete list in one place Line 320: // TODO: Inject global dependencies to make this code testable. same here Line 465: lock_guard<mutex> l(backend_map_lock_); if you're trying to assign 100k scan ranges, this is still going to cause contention, because you're effectively single-threading scheduling. also, this map only changes very infrequently. a read lock would be sufficient (and then you can do it outside of the loop). there is a std::shared_mutex, but i think it's c++17. otherwise implement it yourself. Line 553: AssignmentHeap::handle_type handle = node_assignment.backend_heap.push(new_info); you always update backend_heap and backend_handles together (and if you don't something breaks). turn AssignmentInfo into a class and provide a function for the update. Line 1058: // Reset the random backend ranks and global assignment state. use actual var names in comments Line 1067: AssignmentHeap::handle_type handle = global_assignment_.backend_heap.push(new_info); might as well use an inlined c'tor here Line 1078: AssignmentInfo* node_assignment) { either change to context or to query_node_assignment, to differentiate more clearly from the global one. you're not changing node_assignment, why not a const&? also, node is ambiguous (plan node? backend?). if it's a backend, use that term. Line 1079: // This method is called with a list of local backend candidates, so they also must be also dcheck(!data_locations.empty()) Line 1080: // contained in backend_handles and thus in the global_assignment_ heap. what is backend_handles? what is the list of local backend candidates? Line 1081: vector<size_t> candidates; what are these? Line 1084: for (size_t i = 0; i < data_locations.size(); ++i) { why not just int? Line 1089: const BackendAssignmentInfo& heap_data = *handle_it->second; rename variable to reflect its meaning Line 1102: // candidates in-place. more specifically ", and remove all others from 'candidates'" (instead of 'modifying ...') Line 1103: int64_t min_last_assigend = numeric_limits<int64_t>::max(); spelling Line 1108: const BackendAssignmentInfo& heap_data = meaningful name Line 1109: *(global_assignment_.backend_handles[backend_ip]); who locks this? Line 1139: AssignmentInfo* node_assignment) { change to query_assignment to differentiate from global_assignment_ Line 1148: // implies that the node_assignment heap must contain all of the handles, because all of which handles? Line 1149: // backends in the global heap are ordered by the time of their last assignment. If well, primarily by the number of assigned bytes Line 1153: // list in between scheduling runs for multiple scan ranges of the same plan node. i'm not following. Line 1158: // scheduling of multiple scan ranges of the same plan node. does this get handled for global_assignment_? http://gerrit.cloudera.org:8080/#/c/2200/22/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: Line 106: int64_t last_assignment_timestamp; comment that this is a logical timestamp, derived from num_assignments_ Line 141: /// Total number of assignments made during the lifetime of the scheduler. clarify what you mean by assignment (per query, per plan node, per scan range). also you use singular and plural, sometimes side by side (see global_assignment_) Line 356: /// Resolve a host to one of its IP addresses by calling getaddrinfo() and sorting the comment on semantics of result, not how it's implemented Line 371: /// minimum number of assigned bytes. If backends have been assigned equal amounts of i guess this is "with the minimum number of assigned bytes (according to 'context')" Line 378: /// Among backend hosts collocated with 'data_locations', select the one with the collocated? -- 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: 22 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
