Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201607211428.u6LES5bb027033@ip-10-146-233-104.ec2.internal>
Date: Thu, 21 Jul 2016 14:28:05 +0000
From: "Lars Volker (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, dev@impala.incubator.apache.org
CC: Mostafa Mokhtar <mmokhtar@cloudera.com>, anujphadke <aphadke@cloudera.com>,
        Marcel Kornacker <marcel@cloudera.com>,
        Matthew Jacobs <mj@cloudera.com>, Sailesh Mukil <sailesh@cloudera.com>
Reply-To: lv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-CR=5D=28cdh5-trunk=29_IMPALA-2979=3A_Fix_scheduling_on_remote_hosts=0A?=
In-Reply-To: <gerrit.1455696443000.I044f83806fcde820fcb38047cf6b8e780d803858@gerrit.cloudera.org>
References: <gerrit.1455696443000.I044f83806fcde820fcb38047cf6b8e780d803858@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Thu, 21 Jul 2016 14:28:12 -0000

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 <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