Lars Volker has posted comments on this change. Change subject: IMPALA-2979: Fix scheduling on remote hosts ......................................................................
Patch Set 27: (15 comments) Thanks for the review, please see PS28. http://gerrit.cloudera.org:8080/#/c/2200/24/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 439: backend_candidates.push_back(backend_ip); > could you add a todo to that effect to the .h? We had one there already, I changed it to be more specific. Line 439: backend_candidates.push_back(backend_ip); > yes, let's reevaluate the scheduling strategy based on more extensive test Ok. Line 440: } else if (memory_distance == min_distance) { > ? You're right, done. Line 509: // (the root fragment doesn't have a destination) > is that not true? It is, I thought it doesn't help much to comment on this here. I added it back. PS24, Line 1089: total_assignments_-> > why does moving the seed generator into the main scheduler class cause a pr It won't cause a problem to move the seed generator there, as long as the seeds are generated deterministically (which is what rand() does). Eventually we might want to factor out and inject all global dependencies of the scheduler so it becomes more testable. Should we move the call to rand() outside and pass a seed (int) here? Or pass in a functor to generate the random numbers for shuffle? Or keep it as is for now? I'm good with either, none of them will make a difference wrt testability. Line 1090: if (!remote_read) total_local_assignments_->Increment(1); > let's do it as a follow-on (but let's do that right after this gets in). Added a class level todo. http://gerrit.cloudera.org:8080/#/c/2200/27/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 916: BackendList* be_descs = &backend_map_[be_desc.ip_address]; > i'm not a fan of using [] for reading, because that has the potential also Done Line 1100: // Explicitly set the optional fields. > why? The comment didn't make sense, I removed it. http://gerrit.cloudera.org:8080/#/c/2200/25/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: Line 174: /// AssignmentHeap it can be used to look up heap elements by their IP address and > i guess it's good to leave here. Done Line 327: > is this the local backend? local_backend_descriptor_? Done http://gerrit.cloudera.org:8080/#/c/2200/27/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: Line 140: /// Internal structure to track scan range assignments for a backend. This struct is > ah, so this is really for a backend host, not just backend. Yes. Line 141: /// used as the heap element in AddressableAssignmentHeap. > "heap element in and maintained by "? (or is this updated by anyone else?) Done Line 148: int random_rank; > you can also make this const int and then provide a c'tor that sets the ran Good idea, aggregate initialization makes this possible without a c'tor even. Line 247: /// member in AssignmentCtx. > streamline comment Done Line 428: /// Finally scan ranges are considered which do not have an impalad backend running on > sorry, i think it's actually "finally, ..." 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: 27 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-HasComments: Yes
