mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 14669: launchTasks on list of offers
Date Tue, 22 Oct 2013 00:07:09 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message