impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2979: Fix scheduling on remote hosts
Date Tue, 12 Jul 2016 18:30:29 GMT
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 <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