----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51320/#review146501 -----------------------------------------------------------
src/master/master.cpp (line 93) <https://reviews.apache.org/r/51320/#comment213012> 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. ``` src/master/master.cpp (lines 96 - 97) <https://reviews.apache.org/r/51320/#comment212982> switch the order src/master/master.cpp (line 3326) <https://reviews.apache.org/r/51320/#comment212990> 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. src/master/master.cpp (line 3915) <https://reviews.apache.org/r/51320/#comment213011> s/they/all tasks in the group src/master/master.cpp (lines 3932 - 3933) <https://reviews.apache.org/r/51320/#comment212984> new line here src/master/master.cpp (lines 3947 - 3949) <https://reviews.apache.org/r/51320/#comment212985> What about following? ``` error = Error("Failed to authorize task '" + stringify(task.task_id()) + "': " + authorization.failure()); ``` src/master/master.cpp (lines 3949 - 3950) <https://reviews.apache.org/r/51320/#comment212986> new line here src/master/master.cpp (line 3951) <https://reviews.apache.org/r/51320/#comment213001> kill this src/master/master.cpp (lines 3961 - 3963) <https://reviews.apache.org/r/51320/#comment212989> How about: ``` error = Error("Task '" + stringify(task.task_id()) + "' " "is not authorized to launch as user '" + user + "'"); ``` src/master/master.cpp (lines 3963 - 3964) <https://reviews.apache.org/r/51320/#comment212988> new line here src/master/master.cpp (line 3965) <https://reviews.apache.org/r/51320/#comment213002> kill this src/master/master.cpp (lines 4025 - 4026) <https://reviews.apache.org/r/51320/#comment212993> What about ``` "A task within the task group was killed before " "delivery to the executor"); ``` src/master/master.cpp (line 4050) <https://reviews.apache.org/r/51320/#comment212995> how about s/int/size_t? src/master/master.cpp (line 4076) <https://reviews.apache.org/r/51320/#comment212997> kill this? - Guangya Liu 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 > >
