> On 八月 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, line 93 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line93> > > > > I'd prefer that we move this under `using std::xxx`. > > > > ``` > > using std::list; > > using std::set; > > using std::shared_ptr; > > using std::string; > > using std::vector; > > > > using google::protobuf::RepeatedPtrField; > > > > sing process::await; > > using process::wait; // Necessary on some OS's to disambiguate. > > ``` > > Benjamin Mahler wrote: > I think we follow alphabetical sections, no?
If keeping alphabetical order, then the `using process::await;` should goes before `using std::list;`. I saw that in the code, we are always putting `std::xxx` first, then others. One example here: https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L50-L57 > On 八月 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, line 3326 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line3326> > > > > Just a question here: Do we need the `CHECK` here? I think that from > > here we are pretty sure that the operation type here is > > `Offer::Operation::LAUNCH_GROUP` as we already filtered the operations in > > line 3317 and 3318. > > Benjamin Mahler wrote: > Well, I would like the code to fail if someone breaks this, I could use > UNREACHABLE instead: > > ``` > const RepeatedPtrField<TaskInfo>& tasks = [&]() { > if (operation.type() == Offer::Operation::LAUNCH) { > return operation.launch().task_infos(); > } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) { > return operation.launch_group().task_group().tasks(); > } > UNREACHABLE(); > }(); > ``` Good to know, thanks Ben. > On 八月 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, lines 4031-4032 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line4031> > > > > What about > > > > ``` > > "A task within the task group was killed before " > > "delivery to the executor"); > > ``` > > Benjamin Mahler wrote: > I encourage putting the space onto the next line as it is a bit more > readable and leads to fewer mistakes where we miss the space: > > ``` > LOG(INFO) << "Launching task group " << stringify(taskIds) > << " of framework " << *framework > << " with resources " << totalResources > << " on agent " << *slave; > ``` > > here it's easy to see that each line continues from the previous one, > compare this with: > > ``` > LOG(INFO) << "Launching task group " << stringify(taskIds) << " " > << "of framework " << *framework << " " > << "with resources " << totalResources << " " > << "on agent " << *slave; > ``` Good to know this. - Guangya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51320/#review146501 ----------------------------------------------------------- On 八月 23, 2016, 5:11 a.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51320/ > ----------------------------------------------------------- > > (Updated 八月 23, 2016, 5:11 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-6045 > https://issues.apache.org/jira/browse/MESOS-6045 > > > Repository: mesos > > > Description > ------- > > This operation is all-or-nothing, in that all tasks must be > launched together. If the operation fails, all tasks will > fail with TASK_ERROR and the appropriate GROUP reason. > If a task was killed before delivery to the executor, all > tasks are killed. > > > Diffs > ----- > > include/mesos/mesos.proto 53b6547281b23ce9f47c9f1a418b60667fb4f602 > include/mesos/v1/mesos.proto f6b59e156c92a26dd63b11bf403bdba677b9644b > src/master/master.cpp d94a8510c4cee9c010706f79caf27ef4a10b41a8 > src/messages/messages.proto 7b5e24fb1e9baf09ce024daeca90745f380d4c2f > > Diff: https://reviews.apache.org/r/51320/diff/ > > > Testing > ------- > > Added a test in the subsequent patch. > > More tests will be added for all-or-nothing validation / authorization paths. > > > Thanks, > > Benjamin Mahler > >
