Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2979: Fix scheduling on remote hosts ......................................................................
Patch Set 24: (6 comments) http://gerrit.cloudera.org:8080/#/c/2200/24/be/src/scheduling/simple-scheduler.cc File be/src/scheduling/simple-scheduler.cc: Line 1070: { formatting Line 1075: DCHECK(backend_ctx->NumBackends() == assignment_ctx.backend_heap().size()); this is an invariant between two data structures which you didn't point out in the .h file (and which is another indication that maybe there should only be one struct) PS24, Line 1089: std::mt19937 g(rand()) > You'd probably want to use a std::random_device() to seed the generator her do we want some degree of determinism here, in order to make tests deterministic? if so, you could get the seed from SimpleScheduler, at the time you get the backend config pointer. Line 1090: std::shuffle(random_backend_order_.begin(), random_backend_order_.end(), g); hm, you're doing this once per plan node. i'd be interesting to see the timing numbers from a microbenchmark, i'm wondering if there's too much setup overhead per plan node. Line 1110: bool SimpleScheduler::BackendCtx::LookUpBackendIp(const Hostname& hostname, member fn of BackendMaps? http://gerrit.cloudera.org:8080/#/c/2200/24/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: PS24, Line 192: int next_unused_backend_idx_; > Shouldn't this be an atomicInt? describe somewhere how you're keeping track of unused backends -- 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: 24 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
