Lars Volker has posted comments on this change. Change subject: IMPALA-2979: Fix scheduling on remote hosts ......................................................................
Patch Set 27: (3 comments) Thanks for the comments. I added more comments to the code but otherwise didn't change it. Next I'll rebase, run tests and start the GVM. http://gerrit.cloudera.org:8080/#/c/2200/28/be/src/scheduling/simple-scheduler-test.cc File be/src/scheduling/simple-scheduler-test.cc: Line 765: plan_.scan_range_locations(), plan_.referenced_datanodes(), exec_at_coord, > are there no tests for the random_replica flag? We test its behavior, but only through the query options. This assumes that line https://gerrit.cloudera.org/#/c/2200/28/be/src/scheduling/simple-scheduler.cc@397 does what it should. Once the scheduler becomes more testable we should add tests for the query-level ComputeScanRangeAssignment() method, too. Especially when we switch to tracking AssignmentCtx per query, instead of per plan node. http://gerrit.cloudera.org:8080/#/c/2200/27/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 1100: // Explicitly set the optional fields. > well, you are explicitly setting the optional fields. you could also leave No, we need to set those so the execution code in the backend knows what to expect. We should revisit whether those fields need to be optional. Given we see confusion around unexpected remote reads it might make sense to explicitly consider what their values should be. http://gerrit.cloudera.org:8080/#/c/2200/28/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: Line 61: public: > what we need is a microbenchmark for the performance of the scheduler itsel I expanded the comment on this one so we don't lose track. -- 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 <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
