You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2013/10/16 08:15:22 UTC
Review Request 14669: launchTasks on list of offers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
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 9f5e25b
src/master/master.cpp 1bf5d47
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp feea541
Diff: https://reviews.apache.org/r/14669/diff/
Testing
-------
A new test, MasterTest.LaunchCombinedOfferTest, has been added.
This test ensures that:
1) Multiple offers can be used to run a single task (requesting the sum of offer resources).
2) No offers can appear more than once in offer list.
3) Offers cannot span multiple slaves.
$ make check
...
[ RUN ] MasterTest.LaunchCombinedOfferTest
[ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
...
Thanks,
Niklas Nielsen
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp, lines 710-711
> > <https://reviews.apache.org/r/14669/diff/2/?file=365053#file365053line710>
> >
> > If you change the signature we would have backwards compatibility issues. For example if framework writers use old jar with old launchTasks() and mesos libs are upgraded to use the new launchTasks().
> >
> > We should have the deprecated jni method for old launchTasks() for atleast one version before we can kill it.
> >
> > Makes sense?
>
> Niklas Nielsen wrote:
> Definitely! I was struggling with method overloading in JNI, but I'll take a second look at it. The two method signatures (method names are mangled in case of overloading) was generated just fine, but caused LinkError in the JVM anyway.
>
> Vinod Kone wrote:
> Yea. I had trouble getting jni overloading to work too. John's comment on https://reviews.apache.org/r/14415/ might help.
Awesome! Thanks the reference - the problem turned out to be exactly that '$' was mangled incorrectly to '_'. With the unicode tag, it should have been '_00024'.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27071
-----------------------------------------------------------
On Oct. 17, 2013, 6:19 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2013, 6:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp, lines 710-711
> > <https://reviews.apache.org/r/14669/diff/2/?file=365053#file365053line710>
> >
> > If you change the signature we would have backwards compatibility issues. For example if framework writers use old jar with old launchTasks() and mesos libs are upgraded to use the new launchTasks().
> >
> > We should have the deprecated jni method for old launchTasks() for atleast one version before we can kill it.
> >
> > Makes sense?
>
> Niklas Nielsen wrote:
> Definitely! I was struggling with method overloading in JNI, but I'll take a second look at it. The two method signatures (method names are mangled in case of overloading) was generated just fine, but caused LinkError in the JVM anyway.
Yea. I had trouble getting jni overloading to work too. John's comment on https://reviews.apache.org/r/14415/ might help.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27071
-----------------------------------------------------------
On Oct. 17, 2013, 6:19 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2013, 6:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, lines 808-810
> > <https://reviews.apache.org/r/14669/diff/2/?file=365060#file365060line808>
> >
> > Why did you pull this out? I would put it on line #787.
>
> Niklas Nielsen wrote:
> tasks will be added |offerIds| times if we leave it in the loop above.
Same issue opened in more recent review.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27071
-----------------------------------------------------------
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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > include/mesos/scheduler.hpp, lines 238-246
> > <https://reviews.apache.org/r/14669/diff/2/?file=365052#file365052line238>
> >
> > This comment needs to be updated.
> >
> > Haven't looked at the rest of the review, but what are the semantics here?
> >
> > Is each offer in offerIds supposed to be mapped to the corresponding index in the tasks list?
> >
> >
> >
Offers are aggregated into one resource and treated similar to resources from a single offer.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp, lines 710-711
> > <https://reviews.apache.org/r/14669/diff/2/?file=365053#file365053line710>
> >
> > If you change the signature we would have backwards compatibility issues. For example if framework writers use old jar with old launchTasks() and mesos libs are upgraded to use the new launchTasks().
> >
> > We should have the deprecated jni method for old launchTasks() for atleast one version before we can kill it.
> >
> > Makes sense?
Definitely! I was struggling with method overloading in JNI, but I'll take a second look at it. The two method signatures (method names are mangled in case of overloading) was generated just fine, but caused LinkError in the JVM anyway.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 726
> > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line726>
> >
> > Why pull this out into a temporary?
No need. It has been killed :)
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1101-1108
> > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line1101>
> >
> > How about a generic (templated) output operator for vector<T> ?
stringify(vector<>) already exists, so I killed this helper.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 2178
> > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line2178>
> >
> > It seems odd to do this check this late. How about ensuring we have valid offers before processTasks() is called instead?
If offerId appears multiple times in offerIds, without an explicit check, a offer can be tried to removed multiple times which causes a fault.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/sched/sched.cpp, lines 808-810
> > <https://reviews.apache.org/r/14669/diff/2/?file=365060#file365060line808>
> >
> > Why did you pull this out? I would put it on line #787.
tasks will be added |offerIds| times if we leave it in the loop above.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/master/master.hpp, lines 171-173
> > <https://reviews.apache.org/r/14669/diff/2/?file=365056#file365056line171>
> >
> > Can you update the comment?
This method has been merged into launchTasks() and I killed the comment.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1110-1149
> > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line1110>
> >
> > This function needs some refactoring now that it takes vector<Offer>.
> >
> > This is what I propose we do:
> >
> > // Merge processTasks() in to launchTasks().
> >
> > launchTasks()
> > {
> > // Validate offers. Perhaps we can write OfferValidators like we do for task validators.
> >
> > // Validate tasks.
> >
> > // launchTask().
> > }
> >
> > I suspect this will also eliminate/simplify some of the helpers that were added (e.g., reportInvalidOffers(), combinedResources())
> >
> > Does that make sense?
Definitely! I gave it a shot and merged in processTasks() and introduced OfferVisitor scheme.
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/python/native/mesos_scheduler_driver_impl.cpp, lines 414-416
> > <https://reviews.apache.org/r/14669/diff/2/?file=365059#file365059line414>
> >
> > How is this possible?
This was copied from the task list generation just below. You are right - we should be able to trust the index (as that is the only case we get a NULL pointer from PyList_GetItem. Would you suggest we remove both tests?
> On Oct. 16, 2013, 11:18 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 677
> > <https://reviews.apache.org/r/14669/diff/2/?file=365057#file365057line677>
> >
> > s/getCombinedResources/resources/ ?
resources()/getCombinedResources() has been killed.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27071
-----------------------------------------------------------
On Oct. 17, 2013, 6:10 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2013, 6:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> Diff: https://reviews.apache.org/r/14669/diff/
>
>
> Testing
> -------
>
> A new test, MasterTest.LaunchCombinedOfferTest, has been added.
> This test ensures that:
> 1) Multiple offers can be used to run a single task (requesting the sum of offer resources).
> 2) No offers can appear more than once in offer list.
> 3) Offers cannot span multiple slaves.
>
> $ make check
> ...
> [ RUN ] MasterTest.LaunchCombinedOfferTest
> [ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
> ...
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27071
-----------------------------------------------------------
include/mesos/scheduler.hpp
<https://reviews.apache.org/r/14669/#comment52696>
This comment needs to be updated.
Haven't looked at the rest of the review, but what are the semantics here?
Is each offer in offerIds supposed to be mapped to the corresponding index in the tasks list?
src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp
<https://reviews.apache.org/r/14669/#comment52697>
If you change the signature we would have backwards compatibility issues. For example if framework writers use old jar with old launchTasks() and mesos libs are upgraded to use the new launchTasks().
We should have the deprecated jni method for old launchTasks() for atleast one version before we can kill it.
Makes sense?
src/java/src/org/apache/mesos/MesosSchedulerDriver.java
<https://reviews.apache.org/r/14669/#comment52698>
You probably want to reorder these two.
src/master/master.hpp
<https://reviews.apache.org/r/14669/#comment52700>
Can you update the comment?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52709>
s/getCombinedResources/resources/ ?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52702>
s/Attempt to get resources from empty/Empty/ ?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52703>
s/list/list./
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52704>
Pull this down after the framework and slave checks. That way you won't be unnecessarily putting something in the map if you are going to error out immediately.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52706>
Is this even possible? A framework cannot possibly know offer ids of offers that were not sent to it?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52708>
Why pull this out into a temporary?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52714>
How about a generic (templated) output operator for vector<T> ?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52711>
new line
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52758>
This function needs some refactoring now that it takes vector<Offer>.
This is what I propose we do:
// Merge processTasks() in to launchTasks().
launchTasks()
{
// Validate offers. Perhaps we can write OfferValidators like we do for task validators.
// Validate tasks.
// launchTask().
}
I suspect this will also eliminate/simplify some of the helpers that were added (e.g., reportInvalidOffers(), combinedResources())
Does that make sense?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52715>
Why not take Framework* as parameter?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52716>
s/combinedResourcesTry/totalResources/
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52717>
It seems odd to do this check this late. How about ensuring we have valid offers before processTasks() is called instead?
src/python/native/mesos_scheduler_driver_impl.cpp
<https://reviews.apache.org/r/14669/#comment52718>
s/offerArgument/offerIdsObj/
src/python/native/mesos_scheduler_driver_impl.cpp
<https://reviews.apache.org/r/14669/#comment52719>
How is this possible?
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment52725>
Not yours, but can you log the slave id?
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment52726>
Not yours, but can you log the offer id?
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment52720>
Why did you pull this out? I would put it on line #787.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment52733>
consider
Resources fullSlave = halfSlave + halfSlave;
Resources twoSlaves = fullSlave + fullSlave;
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment52734>
s/notified with/notified/
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment52735>
s/offer/offer./
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment52737>
You could also check the resources match what you expect.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment52738>
You could also check resources match what you expect.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment52739>
Split this into its own test. This test is huge!
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment52740>
Ditto. Split this into its own test.
- Vinod Kone
On Oct. 16, 2013, 6:15 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2013, 6:15 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> Diff: https://reviews.apache.org/r/14669/diff/
>
>
> Testing
> -------
>
> A new test, MasterTest.LaunchCombinedOfferTest, has been added.
> This test ensures that:
> 1) Multiple offers can be used to run a single task (requesting the sum of offer resources).
> 2) No offers can appear more than once in offer list.
> 3) Offers cannot span multiple slaves.
>
> $ make check
> ...
> [ RUN ] MasterTest.LaunchCombinedOfferTest
> [ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
> ...
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27106
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment52767>
This is a bit tricky. If an offer appears more than once in offerIds, it will try to remove same offer more than once which causes a fault.
src/python/native/mesos_scheduler_driver_impl.cpp
<https://reviews.apache.org/r/14669/#comment52766>
This check was copied from the tasks list processing just below. PyList_GetItem only returns NULL when i is out of bounds, so we should be able to remove both checks.
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment52764>
Each task will be added |offerIds| times if add_tasks remains in the loop.
- Niklas Nielsen
On Oct. 17, 2013, 6:10 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2013, 6:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> Diff: https://reviews.apache.org/r/14669/diff/
>
>
> Testing
> -------
>
> A new test, MasterTest.LaunchCombinedOfferTest, has been added.
> This test ensures that:
> 1) Multiple offers can be used to run a single task (requesting the sum of offer resources).
> 2) No offers can appear more than once in offer list.
> 3) Offers cannot span multiple slaves.
>
> $ make check
> ...
> [ RUN ] MasterTest.LaunchCombinedOfferTest
> [ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
> ...
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 18, 2013, 12:04 a.m., Vinod Kone wrote:
> > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp, line 761
> > <https://reviews.apache.org/r/14669/diff/6/?file=366096#file366096line761>
> >
> > As discussed on IRC, this is not enough to be backwards compatible.
> >
> > After discussing with BenH and BenM, this is what I propose:
> >
> > Change offer_id from required to optional in LaunchTasksMessage.
> >
> > Added a new repeated offer_ids field in LaunchTasksMessage.
> >
> > We still need 2 overloads in the JNI for the old and new launchTasks().
> >
> > In sched.cpp, we update launchTasks() to take vector<OfferIds>
> >
> > Both the JNI methods call the new launchTasks().
> >
> > Makes sense?
Sounds good!
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27166
-----------------------------------------------------------
On Oct. 17, 2013, 9:58 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2013, 9:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27166
-----------------------------------------------------------
src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp
<https://reviews.apache.org/r/14669/#comment52852>
As discussed on IRC, this is not enough to be backwards compatible.
After discussing with BenH and BenM, this is what I propose:
Change offer_id from required to optional in LaunchTasksMessage.
Added a new repeated offer_ids field in LaunchTasksMessage.
We still need 2 overloads in the JNI for the old and new launchTasks().
In sched.cpp, we update launchTasks() to take vector<OfferIds>
Both the JNI methods call the new launchTasks().
Makes sense?
- Vinod Kone
On Oct. 17, 2013, 9:58 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2013, 9:58 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27651
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
> On Nov. 14, 2013, 6:12 p.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 1468
> > <https://reviews.apache.org/r/14669/diff/10-14/?file=372806#file372806line1468>
> >
> > You are right, the check is redundant as we fail early if the offer list is empty.
> > Even if slave validation fails, we report and abort before getting to the task validation.
> > I had a bad task for some reason, not checking before using .get() but I'll pull it out.
>
> Niklas Nielsen wrote:
> * bad task -> bad taste
Sorry, maybe I wasn't clear. If you are sure that slaveId cannot be none then there should be a CHECK.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review28879
-----------------------------------------------------------
On Nov. 14, 2013, 6:49 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 6:49 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 380e087
> 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 5b0ca39
> src/master/master.hpp e377af8
> src/master/master.cpp 8e14a07
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3abe72f
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Nov. 14, 2013, 6:12 p.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 1468
> > <https://reviews.apache.org/r/14669/diff/10-14/?file=372806#file372806line1468>
> >
> > You are right, the check is redundant as we fail early if the offer list is empty.
> > Even if slave validation fails, we report and abort before getting to the task validation.
> > I had a bad task for some reason, not checking before using .get() but I'll pull it out.
* bad task -> bad taste
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review28879
-----------------------------------------------------------
On Nov. 13, 2013, 10:19 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 10:19 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 380e087
> 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 5b0ca39
> src/master/master.hpp e377af8
> src/master/master.cpp 8e14a07
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3abe72f
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review28879
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment55945>
You are right, the check is redundant as we fail early if the offer list is empty.
Even if slave validation fails, we report and abort before getting to the task validation.
I had a bad task for some reason, not checking before using .get() but I'll pull it out.
- Niklas Nielsen
On Nov. 13, 2013, 10:19 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 10:19 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 380e087
> 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 5b0ca39
> src/master/master.hpp e377af8
> src/master/master.cpp 8e14a07
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3abe72f
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review28888
-----------------------------------------------------------
Ship it!
Good stuff!
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment55956>
May be nice to LOG(WARNING) here
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment55957>
newline before this line?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment55959>
None()?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment55960>
CHECK_NOTNULL returns the pointer, so we could do:
Slave* slave = CHECK_NOTNULL(getSlave(slaveId.get()));
Up to you, whichever you feel is cleaner :)
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment55961>
None()?
- Ben Mahler
On Nov. 14, 2013, 6:49 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 6:49 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 380e087
> 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 5b0ca39
> src/master/master.hpp e377af8
> src/master/master.cpp 8e14a07
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3abe72f
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review29127
-----------------------------------------------------------
Sorry for the delay on this. Mind rebasing it? I will get this committed today. Thanks.
- Vinod Kone
On Nov. 14, 2013, 10:31 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 10:31 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 380e087
> 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 5b0ca39
> src/master/master.hpp e377af8
> src/master/master.cpp abab6ce
> src/messages/messages.proto 71f68a0
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3abe72f
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Jan. 16, 2014, 10:34 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 1427-1445
> > <https://reviews.apache.org/r/14669/diff/18/?file=392804#file392804line1427>
> >
> > Unfortunately, the offer visitors won't be invoked if offerIds is empty.
> >
> > Also, the visitors can't be passed a reference to a slave or an offer if the validity/out-lived check is written as a visitor.
> >
> > Would you prefer to only pass a framework reference and pointer to the master and do master->getOffer() & master->getSlave() in every visitor?
> >
I do have a version ready in case of the latter. You are right, the launchTask code looks much cleaner but it introduces more code in the visitors. Also, Master::getSlave() and Master::getOffer() needs to be made available to the visitors. I got around it by making the OfferVisitor a friend of Master and have convenienve methods to let sub-classed visitors access Master::getSlave() and Master::getOffer().
Let me know if that is the way we want to go, and I'll update the diff.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review32047
-----------------------------------------------------------
On Jan. 16, 2014, 9:27 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2014, 9:27 a.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 8063997
> 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 5b0ca39
> src/master/master.hpp 18a6cc4
> src/master/master.cpp 008033e
> src/messages/messages.proto 1f264d5
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp f9028e8
> src/tests/master_tests.cpp d34450b
> src/tests/resource_offers_tests.cpp 9beb949
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Jan. 16, 2014, 10:34 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 1427-1445
> > <https://reviews.apache.org/r/14669/diff/18/?file=392804#file392804line1427>
> >
> > Unfortunately, the offer visitors won't be invoked if offerIds is empty.
> >
> > Also, the visitors can't be passed a reference to a slave or an offer if the validity/out-lived check is written as a visitor.
> >
> > Would you prefer to only pass a framework reference and pointer to the master and do master->getOffer() & master->getSlave() in every visitor?
> >
>
> Niklas Nielsen wrote:
> I do have a version ready in case of the latter. You are right, the launchTask code looks much cleaner but it introduces more code in the visitors. Also, Master::getSlave() and Master::getOffer() needs to be made available to the visitors. I got around it by making the OfferVisitor a friend of Master and have convenienve methods to let sub-classed visitors access Master::getSlave() and Master::getOffer().
>
> Let me know if that is the way we want to go, and I'll update the diff.
Ping.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review32047
-----------------------------------------------------------
On Jan. 16, 2014, 4:05 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2014, 4:05 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 8063997
> 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 5b0ca39
> src/master/master.hpp 18a6cc4
> src/master/master.cpp 008033e
> src/messages/messages.proto 1f264d5
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp f9028e8
> src/tests/master_tests.cpp d34450b
> src/tests/resource_offers_tests.cpp 9beb949
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review32047
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment60761>
Unfortunately, the offer visitors won't be invoked if offerIds is empty.
Also, the visitors can't be passed a reference to a slave or an offer if the validity/out-lived check is written as a visitor.
Would you prefer to only pass a framework reference and pointer to the master and do master->getOffer() & master->getSlave() in every visitor?
- Niklas Nielsen
On Jan. 16, 2014, 9:27 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2014, 9:27 a.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 8063997
> 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 5b0ca39
> src/master/master.hpp 18a6cc4
> src/master/master.cpp 008033e
> src/messages/messages.proto 1f264d5
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp f9028e8
> src/tests/master_tests.cpp d34450b
> src/tests/resource_offers_tests.cpp 9beb949
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review32750
-----------------------------------------------------------
Ship it!
Looks great! Thanks for the patience.
Let me know what you think about my comment in sched.cpp.
src/master/master.hpp
<https://reviews.apache.org/r/14669/#comment61711>
s/_offerIds/offerIds/
src/master/master.hpp
<https://reviews.apache.org/r/14669/#comment61712>
lets just do
return executors.contains(frameworkId) &&
executors.get(frameworkId).contains(executorId);
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment61728>
This needs to be rephrased now that we take multiple offers.
maybe s/offer/'launchTasks()'/ ?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment61727>
s/was/is/ ?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment61730>
s/OfferChecker/ValidOfferChecker/ ?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment61731>
why not stringify the offers as you did below?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment61737>
why const?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment61738>
new line.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment61739>
please include offerError.get() here to make it easy for frameworks to understand what is invalid.
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment61724>
This change means, if the scheduler gets deployed first it won't work an old master.
This is a bit concerning especially because it is not going to work even if it is using the old api.
One solution is to have both old and new versions of launchTasks() in the scheduler process.
Thoughts?
- Vinod Kone
On Jan. 17, 2014, 12:05 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 12:05 a.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 8063997
> 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 5b0ca39
> src/master/master.hpp 18a6cc4
> src/master/master.cpp 008033e
> src/messages/messages.proto 1f264d5
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp f9028e8
> src/tests/master_tests.cpp d34450b
> src/tests/resource_offers_tests.cpp 9beb949
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Jan. 24, 2014, 6:24 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Addressed Vinod's comments.
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 (updated)
-----
include/mesos/scheduler.hpp 8063997
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 5b0ca39
src/master/master.hpp 99b8181
src/master/master.cpp c7d9186
src/messages/messages.proto 1f264d5
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp f9028e8
src/tests/master_tests.cpp f1486ce
src/tests/resource_offers_tests.cpp 9beb949
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review32773
-----------------------------------------------------------
Ship it!
Lets get this committed.
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment61745>
s/transition/upgrade/
s/new frameworks/frameworks using new driver/
- Vinod Kone
On Jan. 25, 2014, 2:01 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2014, 2:01 a.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 8063997
> 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 5b0ca39
> src/master/master.hpp 99b8181
> src/master/master.cpp c7d9186
> src/messages/messages.proto 1f264d5
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp f9028e8
> src/tests/master_tests.cpp f1486ce
> src/tests/resource_offers_tests.cpp 9beb949
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Jan. 24, 2014, 6:01 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Addressed Vinod's comments.
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 (updated)
-----
include/mesos/scheduler.hpp 8063997
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 5b0ca39
src/master/master.hpp 99b8181
src/master/master.cpp c7d9186
src/messages/messages.proto 1f264d5
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp f9028e8
src/tests/master_tests.cpp f1486ce
src/tests/resource_offers_tests.cpp 9beb949
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Jan. 16, 2014, 4:05 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Moved additional checks into offer visitors.
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 (updated)
-----
include/mesos/scheduler.hpp 8063997
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 5b0ca39
src/master/master.hpp 18a6cc4
src/master/master.cpp 008033e
src/messages/messages.proto 1f264d5
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp f9028e8
src/tests/master_tests.cpp d34450b
src/tests/resource_offers_tests.cpp 9beb949
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Jan. 16, 2014, 9:27 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Rebased and addressed most of BenM's comments. Will follow up with patch addressing last issue.
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 (updated)
-----
include/mesos/scheduler.hpp 8063997
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 5b0ca39
src/master/master.hpp 18a6cc4
src/master/master.cpp 008033e
src/messages/messages.proto 1f264d5
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp f9028e8
src/tests/master_tests.cpp d34450b
src/tests/resource_offers_tests.cpp 9beb949
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review30069
-----------------------------------------------------------
Hey Nik, a few points below:
1. My only significant comment in this review is that launchTasks is perhaps more complicated than it needs to be, in that it is performing validation that could be delegated to offer validators that you've added here. This will remove the additional code sending TASK_LOST as well as the explicit use of an OfferError. Let me know what you think, I asked benh to take a look at this change as well to get some more opinions here.
2. Can you add a fix version for 0.17.0 on MESOS-749?
3. I would love to see a part 2 for this change where the java / python / C++ test frameworks use the new API call. This will ensure our language bindings work for the new call.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment57601>
Can this be const?
Can framework and slave be const references?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment57603>
Looks like ::some is not needed given the implicit constructor for option. Not yours, but seems like a good time to clean this up given we've introduced other visitors.
Ditto below.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment57602>
s/TaskInfoError::none()/None()/? (Not yours, but good time for a cleanup).
Ditto below.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment57605>
Is this constructor needed?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment57610>
getOffer should not be returning the offer if the slave was disconnected, see Master::exited
This can be CHECK(!slave.disconnected), is validation an effort to be operationally safer than a CHECK?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment57608>
Should this be printing offerId? Or perhaps conditionally printing:
<< stringify(offerIds.empty() ? offerId : stringify(offerIds))
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment57611>
Looks like this case could be an OfferError that gets verified using an OfferVisitor.
If we pass a pointer to the Master (see Slave::Framework / Slave::Executor in slave.cpp), then we can have the OfferVisitors enforce this case here (no offers), as well as the cases below (offer is no longer valid, and offer outlived slave).
After validation, the master code here would be able to assume the request is valid, thus moving the validation details outside of Master::launchTasks.
Does this seem workable? It would be nice to simplify launchTasks in favor of making better validators, thoughts?
- Ben Mahler
On Dec. 2, 2013, 7:34 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2013, 7:34 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 161cc65
> 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 5b0ca39
> src/master/master.hpp a7bf963
> src/master/master.cpp 4f4db93
> src/messages/messages.proto 1f264d5
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp b958435
> src/tests/master_tests.cpp d34450b
> src/tests/resource_offers_tests.cpp 9beb949
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Dec. 2, 2013, 7:34 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
2nd rebase
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 (updated)
-----
include/mesos/scheduler.hpp 161cc65
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 5b0ca39
src/master/master.hpp a7bf963
src/master/master.cpp 4f4db93
src/messages/messages.proto 1f264d5
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp b958435
src/tests/master_tests.cpp d34450b
src/tests/resource_offers_tests.cpp 9beb949
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Nov. 19, 2013, 10:11 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Rebased to master.
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 (updated)
-----
include/mesos/scheduler.hpp 161cc65
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 5b0ca39
src/master/master.hpp c86c1f1
src/master/master.cpp f65b344
src/messages/messages.proto 1f264d5
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 51f95bb
src/tests/master_tests.cpp 37ee7a0
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Nov. 14, 2013, 10:31 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Addressed Ben's latest comments and reintroduced CHECK()
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 (updated)
-----
include/mesos/scheduler.hpp 380e087
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 5b0ca39
src/master/master.hpp e377af8
src/master/master.cpp abab6ce
src/messages/messages.proto 71f68a0
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 3abe72f
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Nov. 14, 2013, 6:49 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Removed redundant CHECK()
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 (updated)
-----
include/mesos/scheduler.hpp 380e087
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 5b0ca39
src/master/master.hpp e377af8
src/master/master.cpp 8e14a07
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 3abe72f
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review28877
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment55943>
Would the check fail if the offers list is empty? If yes, this shouldn't be a check.
- Vinod Kone
On Nov. 13, 2013, 10:19 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 10:19 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 380e087
> 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 5b0ca39
> src/master/master.hpp e377af8
> src/master/master.cpp 8e14a07
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3abe72f
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Nov. 13, 2013, 10:19 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Addressed Ben Mahler's latest review.
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 (updated)
-----
include/mesos/scheduler.hpp 380e087
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 5b0ca39
src/master/master.hpp e377af8
src/master/master.cpp 8e14a07
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 3abe72f
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > Cool! Sorry for taking so long to look at this!
> >
> > We should dogfood our own API changes, can you send out a subsequent change for all of the test frameworks to update any calls to launchTasks?
Thanks for the review Ben!
You bet, I'll follow up with a new review request.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > include/mesos/scheduler.hpp, lines 242-249
> > <https://reviews.apache.org/r/14669/diff/13/?file=373620#file373620line242>
> >
> > Just an observation, this may prove to be slightly confusing for the users of this API.
> >
> > If they don't hold on to offers, they can't possibly place two offers into a vector and make a valid call to launchTasks. Perhaps we should clarify that the only way for a framework to have multiple offers for the same slave is if they're holding on to offers.
> >
> > Seems good for now, in the future, having a document in the /docs/ folder that explains the offer model in Mesos would be great for framework developers.
Noted.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/java/src/org/apache/mesos/SchedulerDriver.java, lines 110-122
> > <https://reviews.apache.org/r/14669/diff/13/?file=373623#file373623line110>
> >
> > Curious as to why you used @deprecated earlier but you're not using @deprecated on these two?
No particular reason. The new patch use @deprecated for those too.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1021
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1021>
> >
> > Have you seen post-reviews.py? It will allow you to break apart your diffs into multiple patches (for example it looks like you moved the visitors above launchTasks, so in this diff it's difficult to see what's changed in the visitors).
> >
> > As an example, you could break these into two commits and send two separate reviews very easily with post-reviews.py. :)
Sorry about that. In this case, processTasks() was merged into launchTasks() so the task visitors either needed to be prototyped or moved above launchTasks.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1250-1253
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1250>
> >
> > Does the typedef prevent you from directly returning the string, using the implicit Option<T> constructor?
> >
> > return "Offer " + ...;
> >
> > Ditto for SlaveChecker and UniqueOfferIDChecker.
Ah, right - good point! I was too heavily influenced by the old visitors :)
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1256
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1256>
> >
> > Does this work with the typedef?
> >
> > return None();
> >
> > Ditto for SlaveChecker and UniqueOfferIDChecker.
Thanks! done
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1263
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1263>
> >
> > This sounds like UniqueSlaveChecker, although I'm surprised to see this checking for null pointers :(
> >
> > Can this take const& for all of these?
> >
> > Looking at TaskInfoChecker, even though it takes pointers, none of the visitors are checking for NULL and it seems they should be taking const references instead. Feel free to make that change here or leave as is, but for the OfferVisitors can you take const& for the arguments?
I changed the arguments to be const references instead and removed the null check.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 1323-1328
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1323>
> >
> > What if we broke up the validation steps?
> >
> > First the offers are validated, then cleanup the offer visitors after this point (rather than at the very end with the task visitors). If there was an error, we have to remove the offers and return.
> >
> > Now, we validate the tasks, launching the valid ones. Then, always cleanup the task visitors / allocator / offers.
> >
> > Let's try to flatten the control flow a bit and avoid the nested break statements.
Good point, I moved the offer validation tear-down up and flattened the control flow. I haven't been able to get rid of the nested break unfortunately.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1335
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1335>
> >
> > Definitely! Maybe a better place for this kind of TODO is on install() so that others find it more easily?
I will follow up with a subsequent patch with this todo.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1380
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1380>
> >
> > Let's handle the NULL slave here instead? Would be great if the Visitor can take const references instead of pointers.
Yep - done.
> On Nov. 4, 2013, 8:37 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1521
> > <https://reviews.apache.org/r/14669/diff/13/?file=373625#file373625line1521>
> >
> > Can you add a CHECK_NOTNULL in removeOffer()?
CHECK_NOTNULL will fail if you give a list of offers with duplicates. I thought it would be more graceful to ignore it. Do you still think we should assert?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review28129
-----------------------------------------------------------
On Nov. 13, 2013, 10:19 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2013, 10:19 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 380e087
> 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 5b0ca39
> src/master/master.hpp e377af8
> src/master/master.cpp 8e14a07
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 3abe72f
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review28129
-----------------------------------------------------------
Cool! Sorry for taking so long to look at this!
We should dogfood our own API changes, can you send out a subsequent change for all of the test frameworks to update any calls to launchTasks?
include/mesos/scheduler.hpp
<https://reviews.apache.org/r/14669/#comment54715>
Just an observation, this may prove to be slightly confusing for the users of this API.
If they don't hold on to offers, they can't possibly place two offers into a vector and make a valid call to launchTasks. Perhaps we should clarify that the only way for a framework to have multiple offers for the same slave is if they're holding on to offers.
Seems good for now, in the future, having a document in the /docs/ folder that explains the offer model in Mesos would be great for framework developers.
src/java/src/org/apache/mesos/SchedulerDriver.java
<https://reviews.apache.org/r/14669/#comment54716>
Curious as to why you used @deprecated earlier but you're not using @deprecated on these two?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54717>
Have you seen post-reviews.py? It will allow you to break apart your diffs into multiple patches (for example it looks like you moved the visitors above launchTasks, so in this diff it's difficult to see what's changed in the visitors).
As an example, you could break these into two commits and send two separate reviews very easily with post-reviews.py. :)
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54721>
s/all together/altogether/
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54722>
Does the typedef prevent you from directly returning the string, using the implicit Option<T> constructor?
return "Offer " + ...;
Ditto for SlaveChecker and UniqueOfferIDChecker.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54723>
Does this work with the typedef?
return None();
Ditto for SlaveChecker and UniqueOfferIDChecker.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54726>
This sounds like UniqueSlaveChecker, although I'm surprised to see this checking for null pointers :(
Can this take const& for all of these?
Looking at TaskInfoChecker, even though it takes pointers, none of the visitors are checking for NULL and it seems they should be taking const references instead. Feel free to make that change here or leave as is, but for the OfferVisitors can you take const& for the arguments?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54724>
" uses slave " + stringify(_slave->id) " and slave " + stringify(slave->id);
Let's not say "expected" here since we're not expecting a particular slave, but rather that only one slave is used.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54725>
" uses slave " + stringify(_slave->id) " and slave " + stringify(slave->id);
Let's not say "expected" here since we're not expecting a particular slave, but rather that only one slave is used.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54727>
Looks like an Option<SlaveID> be sufficient here instead of storing the pointer?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54744>
What if we broke up the validation steps?
First the offers are validated, then cleanup the offer visitors after this point (rather than at the very end with the task visitors). If there was an error, we have to remove the offers and return.
Now, we validate the tasks, launching the valid ones. Then, always cleanup the task visitors / allocator / offers.
Let's try to flatten the control flow a bit and avoid the nested break statements.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54728>
Can you deal with the NULL case first to avoid having so much nesting?
if (framework == NULL) {
return; // We ignore this request.
}
vector<OfferID> offerIds;
// Rest of your code.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54730>
Definitely! Maybe a better place for this kind of TODO is on install() so that others find it more easily?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54734>
const OfferID& ?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54735>
Let's handle the NULL slave here instead? Would be great if the Visitor can take const references instead of pointers.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54736>
Does this fit on one line?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54731>
Seems like using Shared or Owned here would eliminate the need for manual memory cleanup here.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment54732>
Can you add a CHECK_NOTNULL in removeOffer()?
src/messages/messages.proto
<https://reviews.apache.org/r/14669/#comment54718>
optional OfferID offer_id = 2; // Deprecated.
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment54720>
Can you remove this TODO? Let's keep it simple here. If this proves to be a performance issue then we can consider an alternative approach, but my hunch is that the overwriting is simple, clean, and ok for now.
- Ben Mahler
On Oct. 29, 2013, 7:12 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 7:12 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 29, 2013, 7:12 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Forgot your request for task ids in error descriptions (in sched.cpp)
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 (updated)
-----
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 29, 2013, 6:58 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Added todo for savedSlavePids issue per discussion above.
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 (updated)
-----
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 29, 2013, 5:27 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Error propagation reworked per discussion in previous review.
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 (updated)
-----
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
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.
Changes
-------
Addressed most of Vinod's comments.
There are two outstanding issues (in sched.cpp and master.cpp) which are still in flux (see last comment).
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 (updated)
-----
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27583
-----------------------------------------------------------
src/java/src/org/apache/mesos/MesosSchedulerDriver.java
<https://reviews.apache.org/r/14669/#comment53542>
Reorder these alphabetically.
src/java/src/org/apache/mesos/SchedulerDriver.java
<https://reviews.apache.org/r/14669/#comment53543>
Update the comment.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53544>
Can you also include the expected slave id?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53545>
include the invalid resource in the error string?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53546>
Not yours, but can you include the slave id here?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53547>
s/is/are/
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53548>
include the expected framework id.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53549>
include slave id.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53552>
s/sharedSlave/slave/
You can name the 'slave' argument above as '_slave'
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53555>
Why not 'break' out of the for loop here?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53558>
CHECK_NOTNULL(slave);
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment53562>
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.
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment53560>
not yours, but can you print the task id?
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment53561>
not yours, but can you print the task id?
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment53563>
kill this per comment above.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53564>
thank you!
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53565>
Can you just use 'fullSlave' here?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53575>
s/slave1/slave/
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53566>
you can do 'resources1.cpus()'
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53567>
you can do 'resources1.mem()'
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53569>
ditto.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53570>
ditto.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53571>
s/combinedOffers1/combinedOffers/
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53572>
Great test!
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53579>
this seems unused expect below. Can you kill it and explicitly define 'fullSlave'?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53573>
ditto. use 'fullSlave'
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53574>
ditto
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53577>
ditto.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53576>
new line.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53580>
ditto.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/14669/#comment53578>
ditto.
- Vinod Kone
On Oct. 22, 2013, 5:37 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2013, 5:37 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 9f5e25b
> src/master/master.cpp acbb137
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp bf790d2
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 28, 2013, 10:10 p.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 1398
> > <https://reviews.apache.org/r/14669/diff/9/?file=369195#file369195line1398>
> >
> > 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?
>
> Vinod Kone wrote:
> I'm confused. There is already a if(oferError.isSome()) check on #1414. Would that not catch it?
>
> Niklas Nielsen wrote:
> Sorry for not being clear. What I mean is that we could do something like this instead:
>
> foreach (OfferID offerId, offerIds) {
> Offer* offer = getOffer(offerId);
> if (offer == NULL) {
> offerError = OfferError::some(
> "Offer " + stringify(offerId) + " is no longer valid");
> break;
> }
>
> Slave* slave = getSlave(offer->slave_id());
> foreach (OfferVisitor* visitor, offerVisitors) {
> offerError = (*visitor)(
> offer,
> framework,
> slave);
>
> if (offerError.isSome()) {
> break;
> }
> }
>
> // Abort offer and task processing if any offer validation failed.
> if (offerError.isSome()) {
> break;
> }
>
> totalResources += offer->resources();
> }
>
> if (offerError.isSome()) {
> LOG(WARNING) << "Failed to validate offer " << offerId
> << " : " << offerError.get();
>
> foreach (const TaskInfo& task, tasks) {
> const StatusUpdate& update = protobuf::createStatusUpdate(
> framework->id,
> task.slave_id(),
> task.task_id(),
> TASK_LOST,
> "Task launched with invalid offers");
>
> LOG(INFO) << "Sending status update " << update
> << " for launch task attempt on invalid offers: "
> << stringify(offerIds);
> StatusUpdateMessage message;
> message.mutable_update()->CopyFrom(update);
> send(framework->pid, message);
> }
> }
>
> Does that look better? Should achieve the same thing.
> We need the last break, as the break in the foreach (OfferVisitor...) loop won't break the outer loop.
Yes. This definitely looks better!
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27626
-----------------------------------------------------------
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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 28, 2013, 10:10 p.m., Niklas Nielsen wrote:
> > src/sched/sched.cpp, lines 784-787
> > <https://reviews.apache.org/r/14669/diff/9/?file=369198#file369198line784>
> >
> > 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 now 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?
>
> Vinod Kone wrote:
> Ah I see. That makes sense.
>
> Niklas Nielsen wrote:
> Should we leave it as it is (overwriting the savedSlavePids entry) or get the new code in which finds and validates common slaveId and slavePid?
I will leave it up to you. Either new code or a TODO to fix up later works for me.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27626
-----------------------------------------------------------
On Oct. 29, 2013, 5:27 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 5:27 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
> On Oct. 28, 2013, 10:10 p.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 1398
> > <https://reviews.apache.org/r/14669/diff/9/?file=369195#file369195line1398>
> >
> > 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?
I'm confused. There is already a if(oferError.isSome()) check on #1414. Would that not catch it?
> On Oct. 28, 2013, 10:10 p.m., Niklas Nielsen wrote:
> > src/sched/sched.cpp, lines 784-787
> > <https://reviews.apache.org/r/14669/diff/9/?file=369198#file369198line784>
> >
> > 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 now 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?
Ah I see. That makes sense.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27626
-----------------------------------------------------------
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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 28, 2013, 10:10 p.m., Niklas Nielsen wrote:
> > src/sched/sched.cpp, lines 784-787
> > <https://reviews.apache.org/r/14669/diff/9/?file=369198#file369198line784>
> >
> > 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 now 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?
>
> Vinod Kone wrote:
> Ah I see. That makes sense.
Should we leave it as it is (overwriting the savedSlavePids entry) or get the new code in which finds and validates common slaveId and slavePid?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27626
-----------------------------------------------------------
On Oct. 29, 2013, 5:27 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 5:27 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 28, 2013, 10:10 p.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 1398
> > <https://reviews.apache.org/r/14669/diff/9/?file=369195#file369195line1398>
> >
> > 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?
>
> Vinod Kone wrote:
> I'm confused. There is already a if(oferError.isSome()) check on #1414. Would that not catch it?
Sorry for not being clear. What I mean is that we could do something like this instead:
foreach (OfferID offerId, offerIds) {
Offer* offer = getOffer(offerId);
if (offer == NULL) {
offerError = OfferError::some(
"Offer " + stringify(offerId) + " is no longer valid");
break;
}
Slave* slave = getSlave(offer->slave_id());
foreach (OfferVisitor* visitor, offerVisitors) {
offerError = (*visitor)(
offer,
framework,
slave);
if (offerError.isSome()) {
break;
}
}
// Abort offer and task processing if any offer validation failed.
if (offerError.isSome()) {
break;
}
totalResources += offer->resources();
}
if (offerError.isSome()) {
LOG(WARNING) << "Failed to validate offer " << offerId
<< " : " << offerError.get();
foreach (const TaskInfo& task, tasks) {
const StatusUpdate& update = protobuf::createStatusUpdate(
framework->id,
task.slave_id(),
task.task_id(),
TASK_LOST,
"Task launched with invalid offers");
LOG(INFO) << "Sending status update " << update
<< " for launch task attempt on invalid offers: "
<< stringify(offerIds);
StatusUpdateMessage message;
message.mutable_update()->CopyFrom(update);
send(framework->pid, message);
}
}
Does that look better? Should achieve the same thing.
We need the last break, as the break in the foreach (OfferVisitor...) loop won't break the outer loop.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27626
-----------------------------------------------------------
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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27626
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53668>
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?
src/sched/sched.cpp
<https://reviews.apache.org/r/14669/#comment53690>
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 now 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?
- Niklas Nielsen
On Oct. 22, 2013, 5:37 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2013, 5:37 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 9f5e25b
> src/master/master.cpp acbb137
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp bf790d2
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 22, 2013, 5:37 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Removed dead reportInvalidOffers prototype.
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 (updated)
-----
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 9f5e25b
src/master/master.cpp acbb137
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp bf790d2
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 22, 2013, 12:02 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
- Replaced base checks with offer visitors.
- Pulled in status update sending into launchTasks.
- Flattened resource aggregation visitor into launchTasks.
- Style fixes.
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 (updated)
-----
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 9f5e25b
src/master/master.cpp acbb137
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp bf790d2
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 21, 2013, 11:31 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Attached JIRA issue.
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 9f5e25b
src/master/master.cpp acbb137
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp bf790d2
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > Also, please attach the JIRA issue to this review.
Cool - done. Thanks for the reviews and patience with this patch btw!
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1230-1231
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1230>
> >
> > Not yours, but can you kill this TODO?
Done.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1286
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1286>
> >
> > Why do you need a hashmap? Is a hashset of OfferID not sufficient?
No need - has been killed.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1315
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1315>
> >
> > I think this is abusing the visitor pattern :)
> >
> > Lets just do offer aggregation inline in launchTasks(), or if you prefer do it in a helper. IIUC, this helper could be used in the future when we implement 'update'.
I have flattened it into launchTasks for now.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1392-1404
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1392>
> >
> > Seems odd to do these checks for only the first offer. Can we use the offer validators do these checks for each offer?
I have added visitors for these checks now.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1487
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1487>
> >
> > This is not really invalid offer right? This is an invalid task.
I have killed reportInvalidOffers and pulled status update sending into launchTasks.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1547
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1547>
> >
> > Is reportInvalidOffers() called from anywhere else? If not, we don't need to pull it out into a helper function?
As mentioned above, I killed reportInvalidOffers and pulled status update sending in.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1439-1440
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1439>
> >
> > This is odd. Why do it here instead of doing it in the if statement above?
I am not sure that I follow: Is it the NULL-check of the offer or breaking on offer validation error? If we don't break, a subsequent (valid) offer check will overwrite previous bad ones.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27254
-----------------------------------------------------------
On Oct. 22, 2013, 12:02 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 22, 2013, 12:02 a.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 9f5e25b
> src/master/master.cpp acbb137
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp bf790d2
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 21, 2013, 6:34 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1439-1440
> > <https://reviews.apache.org/r/14669/diff/7/?file=368247#file368247line1439>
> >
> > This is odd. Why do it here instead of doing it in the if statement above?
>
> Niklas Nielsen wrote:
> I am not sure that I follow: Is it the NULL-check of the offer or breaking on offer validation error? If we don't break, a subsequent (valid) offer check will overwrite previous bad ones.
Same issue opened in more recent review.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27254
-----------------------------------------------------------
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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/#review27254
-----------------------------------------------------------
Also, please attach the JIRA issue to this review.
include/mesos/scheduler.hpp
<https://reviews.apache.org/r/14669/#comment53073>
s/each offer/all offers/
include/mesos/scheduler.hpp
<https://reviews.apache.org/r/14669/#comment53075>
Add a TODO that this is deprecated.
src/java/src/org/apache/mesos/MesosSchedulerDriver.java
<https://reviews.apache.org/r/14669/#comment53076>
Deprecation warning?
Also, the wrapping seems to be incorrect?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53079>
Not yours, but can you kill this TODO?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53080>
new line.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53081>
s/appear/appears/
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53082>
Why do you need a hashmap? Is a hashset of OfferID not sufficient?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53083>
new line.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53085>
s/use/uses/
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53084>
new line.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53087>
I think this is abusing the visitor pattern :)
Lets just do offer aggregation inline in launchTasks(), or if you prefer do it in a helper. IIUC, this helper could be used in the future when we implement 'update'.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53088>
We should send back a TASK_LOST here.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53089>
Seems odd to do these checks for only the first offer. Can we use the offer validators do these checks for each offer?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53094>
If you move the base offer checks above to offer validators, you can s/currentOffer/offer/
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53095>
This is odd. Why do it here instead of doing it in the if statement above?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53091>
This is not really invalid offer right? This is an invalid task.
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53096>
This is also doesn't make much sense now that we accept multiple offers?
src/master/master.cpp
<https://reviews.apache.org/r/14669/#comment53090>
Is reportInvalidOffers() called from anywhere else? If not, we don't need to pull it out into a helper function?
- Vinod Kone
On Oct. 18, 2013, 11:01 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2013, 11:01 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp acbb137
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp bf790d2
>
> 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
>
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 18, 2013, 11:01 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Reintroduce offer_id in LaunchTasksMessage as optional field for backward compatibility, and have offer_ids as additional field.
Master::launchTasks() accepts both new and old message formats.
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 (updated)
-----
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 9f5e25b
src/master/master.cpp acbb137
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp bf790d2
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 17, 2013, 9:58 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Old patch uploaded - trying again.
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 (updated)
-----
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 9f5e25b
src/master/master.cpp 1bf5d47
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp feea541
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 17, 2013, 9:52 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Fix indentation for call wrapping.
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 (updated)
-----
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 9f5e25b
src/master/master.cpp 1bf5d47
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp feea541
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 17, 2013, 7:30 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
launchTasks overloaded in JNI for backward compatibility.
Minor style fix in comment.
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 (updated)
-----
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 9f5e25b
src/master/master.cpp 1bf5d47
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp feea541
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 17, 2013, 6:19 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Test was split into three sub-tests.
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 9f5e25b
src/master/master.cpp 1bf5d47
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp feea541
Diff: https://reviews.apache.org/r/14669/diff/
Testing (updated)
-------
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
Re: Review Request 14669: launchTasks on list of offers
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14669/
-----------------------------------------------------------
(Updated Oct. 17, 2013, 6:10 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
In progress: JNI issue (with method overloading) is still pending.
- Introduced offer visitor scheme similar to task visitors, to validate and aggregate offers.
- Merged processTasks() into launchTasks()
- Split launch tests into sub-tests and validate offer resources.
- Various style fixes.
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 (updated)
-----
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 9f5e25b
src/master/master.cpp 1bf5d47
src/messages/messages.proto a5dded2
src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
src/sched/sched.cpp 824b4b7
src/tests/master_tests.cpp feea541
Diff: https://reviews.apache.org/r/14669/diff/
Testing
-------
A new test, MasterTest.LaunchCombinedOfferTest, has been added.
This test ensures that:
1) Multiple offers can be used to run a single task (requesting the sum of offer resources).
2) No offers can appear more than once in offer list.
3) Offers cannot span multiple slaves.
$ make check
...
[ RUN ] MasterTest.LaunchCombinedOfferTest
[ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
...
Thanks,
Niklas Nielsen
Re: Review Request 14669: launchTasks on list of offers
Posted by Benjamin Hindman <be...@gmail.com>.
Hey Pranay,
We've got SASL support so in theory we should easily be able to do Kerberos via the GSSAPI mechanism of SASL. We currently only "support" (i.e., have tested) shared secrets (the CRAM-MD5 mechanism).
We currently have MESOS-418 in JIRA. This has acted as the "roadmap" for authentication thus far (it might make sense to create another Kerberos specific issue to capture Kerberos specific discussion).
Hope that helps.
Ben.
On Oct 16, 2013, at 7:22 AM, Pranay Tonpay <pr...@impetus.co.in> wrote:
> Hi,
> I am new to this group, so wasn't sure about the protocol to ask this question...
> Is there in road map of Mesos to add something like Kerberos security ?
>
> Thx
> pranay
>
>
> -----Original Message-----
> From: Sam Taha [mailto:tahasam@gmail.com]
> Sent: Wednesday, October 16, 2013 5:40 PM
> To: dev@mesos.apache.org; Niklas Nielsen
> Subject: Re: Review Request 14669: launchTasks on list of offers
>
> This enhancement, is implying it is currently possible for Mesos to deliver, to a single Framework, multiple Offers (with mutually exclusive
> resources) from the same slave?
>
> I am curious why mesos wouldn't merge these resource offers when it delivers the Offers to the Framework. This would seem easier for the Framework to deal with when the Framework is trying to match up its requests to the offers being presented to it.
>
> Thanks,
> Sam Taha
>
> http://www.grandlogic.com
>
>
>
>
> On Wed, Oct 16, 2013 at 2:15 AM, Niklas Nielsen <ni...@qni.dk> wrote:
>
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/14669/
>> -----------------------------------------------------------
>>
>> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>>
>>
>> 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 9f5e25b
>> src/master/master.cpp 1bf5d47
>> src/messages/messages.proto a5dded2
>> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
>> src/sched/sched.cpp 824b4b7
>> src/tests/master_tests.cpp feea541
>>
>> Diff: https://reviews.apache.org/r/14669/diff/
>>
>>
>> Testing
>> -------
>>
>> A new test, MasterTest.LaunchCombinedOfferTest, has been added.
>> This test ensures that:
>> 1) Multiple offers can be used to run a single task (requesting the
>> sum of offer resources).
>> 2) No offers can appear more than once in offer list.
>> 3) Offers cannot span multiple slaves.
>>
>> $ make check
>> ...
>> [ RUN ] MasterTest.LaunchCombinedOfferTest
>> [ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
>> ...
>>
>>
>> Thanks,
>>
>> Niklas Nielsen
>>
>>
>
> ________________________________
>
>
>
>
>
>
> NOTE: This message may contain information that is confidential, proprietary, privileged or otherwise protected by law. The message is intended solely for the named addressee. If received in error, please destroy and notify the sender. Any use of this email is prohibited when received in error. Impetus does not represent, warrant and/or guarantee, that the integrity of this communication has been maintained nor that the communication is free of errors, virus, interception or interference.
RE: Review Request 14669: launchTasks on list of offers
Posted by Pranay Tonpay <pr...@impetus.co.in>.
Hi,
I am new to this group, so wasn't sure about the protocol to ask this question...
Is there in road map of Mesos to add something like Kerberos security ?
Thx
pranay
-----Original Message-----
From: Sam Taha [mailto:tahasam@gmail.com]
Sent: Wednesday, October 16, 2013 5:40 PM
To: dev@mesos.apache.org; Niklas Nielsen
Subject: Re: Review Request 14669: launchTasks on list of offers
This enhancement, is implying it is currently possible for Mesos to deliver, to a single Framework, multiple Offers (with mutually exclusive
resources) from the same slave?
I am curious why mesos wouldn't merge these resource offers when it delivers the Offers to the Framework. This would seem easier for the Framework to deal with when the Framework is trying to match up its requests to the offers being presented to it.
Thanks,
Sam Taha
http://www.grandlogic.com
On Wed, Oct 16, 2013 at 2:15 AM, Niklas Nielsen <ni...@qni.dk> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> Diff: https://reviews.apache.org/r/14669/diff/
>
>
> Testing
> -------
>
> A new test, MasterTest.LaunchCombinedOfferTest, has been added.
> This test ensures that:
> 1) Multiple offers can be used to run a single task (requesting the
> sum of offer resources).
> 2) No offers can appear more than once in offer list.
> 3) Offers cannot span multiple slaves.
>
> $ make check
> ...
> [ RUN ] MasterTest.LaunchCombinedOfferTest
> [ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
> ...
>
>
> Thanks,
>
> Niklas Nielsen
>
>
________________________________
NOTE: This message may contain information that is confidential, proprietary, privileged or otherwise protected by law. The message is intended solely for the named addressee. If received in error, please destroy and notify the sender. Any use of this email is prohibited when received in error. Impetus does not represent, warrant and/or guarantee, that the integrity of this communication has been maintained nor that the communication is free of errors, virus, interception or interference.
Re: Review Request 14669: launchTasks on list of offers
Posted by Sam Taha <ta...@gmail.com>.
Hi Ben,
I see. i was mistakenly assuming that once a got a new set of offers (via
Scheduler.resourceOffers) that the previous set of Offers would be
automatically rescinded or essentially no longer valid for my framework to
use.
Currently my Frameworks processes one set of offers at a time (without
blocking the Scheduler), so this does not sound like a big concern for me
at the moment unless I get into aggregating offers across multiple
Scheduler.resourceOffer calls.
Thanks,
Sam Taha
http://www.grandlogic.com
On Wed, Oct 16, 2013 at 12:52 PM, Benjamin Hindman <
benjamin.hindman@gmail.com> wrote:
> Hi Sam,
>
> > This enhancement, is implying it is currently possible for Mesos to
> > deliver, to a single Framework, multiple Offers (with mutually exclusive
> > resources) from the same slave?
>
> A single collection of offers delivered to a framework via the
> Scheduler.resourceOffers callback will not include more than one offer per
> slave. However, across multiple calls to the framework callback you may
> have two offers for the same slave. If you decline both offers they can be
> aggregated and re-delivered from Mesos. This enhancement let's you
> aggregate them on the scheduler side instead.
>
> Does that help?
>
> Ben.
>
Re: Review Request 14669: launchTasks on list of offers
Posted by Benjamin Hindman <be...@gmail.com>.
Hi Sam,
> This enhancement, is implying it is currently possible for Mesos to
> deliver, to a single Framework, multiple Offers (with mutually exclusive
> resources) from the same slave?
A single collection of offers delivered to a framework via the Scheduler.resourceOffers callback will not include more than one offer per slave. However, across multiple calls to the framework callback you may have two offers for the same slave. If you decline both offers they can be aggregated and re-delivered from Mesos. This enhancement let's you
aggregate them on the scheduler side instead.
Does that help?
Ben.
Re: Review Request 14669: launchTasks on list of offers
Posted by Sam Taha <ta...@gmail.com>.
This enhancement, is implying it is currently possible for Mesos to
deliver, to a single Framework, multiple Offers (with mutually exclusive
resources) from the same slave?
I am curious why mesos wouldn't merge these resource offers when it
delivers the Offers to the Framework. This would seem easier for the
Framework to deal with when the Framework is trying to match up its
requests to the offers being presented to it.
Thanks,
Sam Taha
http://www.grandlogic.com
On Wed, Oct 16, 2013 at 2:15 AM, Niklas Nielsen <ni...@qni.dk> wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14669/
> -----------------------------------------------------------
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> 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 9f5e25b
> src/master/master.cpp 1bf5d47
> src/messages/messages.proto a5dded2
> src/python/native/mesos_scheduler_driver_impl.cpp 059ed5d
> src/sched/sched.cpp 824b4b7
> src/tests/master_tests.cpp feea541
>
> Diff: https://reviews.apache.org/r/14669/diff/
>
>
> Testing
> -------
>
> A new test, MasterTest.LaunchCombinedOfferTest, has been added.
> This test ensures that:
> 1) Multiple offers can be used to run a single task (requesting the sum of
> offer resources).
> 2) No offers can appear more than once in offer list.
> 3) Offers cannot span multiple slaves.
>
> $ make check
> ...
> [ RUN ] MasterTest.LaunchCombinedOfferTest
> [ OK ] MasterTest.LaunchCombinedOfferTest (3043 ms)
> ...
>
>
> Thanks,
>
> Niklas Nielsen
>
>