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