> On Oct. 28, 2013, 1:02 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1398
> > <https://reviews.apache.org/r/14669/diff/9/?file=369195#file369195line1398>
> >
> > Why not 'break' out of the for loop here?
My comments ended up in a separate review, but here it is: "If we do that, we
need to move the
if (offerError.isSome()) {
...
}
block out as well and propagate the error with another:
if (offerError.isSome()) {
break;
}
Or else, an invalid offer won't send a TASK_LOST.
Do you prefer that approach?"
> On Oct. 28, 2013, 1:02 a.m., Vinod Kone wrote:
> > src/sched/sched.cpp, lines 784-787
> > <https://reviews.apache.org/r/14669/diff/9/?file=369198#file369198line784>
> >
> > Why do it this way instead of just doing
> >
> > foreach (const OfferID& offerId, offerIds) {
> > message.add_offer_ids()->MergeFrom(offerId);
> > }
> >
> > Everything else could be same as before.
> >
> > This avoids go through each task for every offer.
Ditto, comment ended up in other review: "We need at least one offer to get the
pid that goes into savedSlavePids. Right now, the savedSlavePids entry is just
overwritten |offers| times which isn't ideal either.
I have a version which splits this into three steps:
1) Find common slave id during the first foreach(... task, tasks)
2) While adding offer ids to the message, find a common UPID for the common
slave.
3) If both slave id and UPID is found, update the savedSlavePids map.
In step 1) and 2), if slave id or pids differ, we report error or abort. This
is more strict than the previous version. What do you think?"
> On Oct. 28, 2013, 1:02 a.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 795
> > <https://reviews.apache.org/r/14669/diff/9/?file=369198#file369198line795>
> >
> > not yours, but can you print the task id?
Depends on issue above.
> On Oct. 28, 2013, 1:02 a.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 799
> > <https://reviews.apache.org/r/14669/diff/9/?file=369198#file369198line799>
> >
> > not yours, but can you print the task id?
Depends on issue above.
> On Oct. 28, 2013, 1:02 a.m., Vinod Kone wrote:
> > src/sched/sched.cpp, lines 810-812
> > <https://reviews.apache.org/r/14669/diff/9/?file=369198#file369198line810>
> >
> > kill this per comment above.
Depends on issue above.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27583
-----------------------------------------------------------
On Oct. 28, 2013, 11:15 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 11:15 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-749
> https://issues.apache.org/jira/browse/MESOS-749
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Running tasks on more than one offer belonging to a single slave can be
> useful in situations with multiple out-standing offers.
>
> This patch extends the usual launchTasks() to accept a vector of OfferIDs.
> The previous launchTasks (accepting a single OfferID) has been kept for
> backward compatibility, but this now calls the new launchTasks() with a
> one-element list.
> This also applied for the JNI and python interfaces, which accepts both
> formats as well.
>
> Offers are verified to belong to the same slave and framework, before
> resources are merged and used.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp fa1ffe8
> src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 9869929
> src/java/src/org/apache/mesos/MesosSchedulerDriver.java ed4b4a3
> src/java/src/org/apache/mesos/SchedulerDriver.java 93aaa54
> src/master/master.hpp 1eba03f
> src/master/master.cpp 1147cc6
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3049096
> src/tests/master_tests.cpp bf790d2
> src/tests/resource_offers_tests.cpp 2864c9a
>
> Diff: https://reviews.apache.org/r/14669/diff/
>
>
> Testing
> -------
>
> Three new tests has been added: LaunchCombinedOfferTest,
> LaunchAcrossSlavesTest and LaunchDuplicateOfferTest
> This test ensures that:
> 1) Multiple offers can be used to run a single task (requesting the sum of
> offer resources).
> 2) Offers cannot span multiple slaves.
> 3) No offers can appear more than once in offer list.
>
> $ make check
> ...
> [ RUN ] MasterTest.LaunchCombinedOfferTest
> [ OK ] MasterTest.LaunchCombinedOfferTest (2010 ms)
> [ RUN ] MasterTest.LaunchAcrossSlavesTest
> [ OK ] MasterTest.LaunchAcrossSlavesTest (3 ms)
> [ RUN ] MasterTest.LaunchDuplicateOfferTest
> [ OK ] MasterTest.LaunchDuplicateOfferTest (3 ms)
> ...
>
>
> Thanks,
>
> Niklas Nielsen
>
>
