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/12/02 20:34:57 UTC
Re: Review Request 14669: launchTasks on list of offers
-----------------------------------------------------------
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>.
> 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
>
>