> On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, line 4056 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line4056> > > > > how about s/int/size_t?
Yes for most containers or loop iterators, but protobuf uses `int` for `.size()` so I use the same type as them. > On Aug. 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"); > > ``` 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; ``` > On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, line 3971 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line3971> > > > > kill this Once I add a newline between the error and reason assignments, the break needs a newline as well or it looks strange. > On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, lines 3967-3969 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line3967> > > > > How about: > > > > ``` > > error = Error("Task '" + stringify(task.task_id()) + "' " > > "is not authorized to launch as user '" + > > user + "'"); > > ``` See my other comment about spaces on the next line. Also, we should avoid opening and closing the quotes on separate lines (as you did with user). > On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, line 3957 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line3957> > > > > kill this Once I add a newline between the error and reason assignments, the break needs a newline as well or it looks strange. > On Aug. 23, 2016, 11:48 a.m., Guangya Liu wrote: > > src/master/master.cpp, lines 3953-3955 > > <https://reviews.apache.org/r/51320/diff/1/?file=1481651#file1481651line3953> > > > > What about following? > > > > ``` > > error = Error("Failed to authorize task '" + > > stringify(task.task_id()) + "': " + > > authorization.failure()); > > ``` See my other comments about quotes across lines and spaces on the next line. > On Aug. 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. 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(); }(); ``` > On Aug. 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. > > ``` I think we follow alphabetical sections, no? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51320/#review146501 ----------------------------------------------------------- On Aug. 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 Aug. 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 > >
