----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66229/#review202529 -----------------------------------------------------------
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp Lines 688 (patched) <https://reviews.apache.org/r/66229/#comment284322> Can you copy the TODO from above? ``` // TODO(anand): Throw java error. ``` src/master/http.cpp Line 952 (original), 952-953 (patched) <https://reviews.apache.org/r/66229/#comment284324> Per discussion, let's keep this as stateless validation, and add the master level validation into the master's handler. src/master/http.cpp Lines 1133 (patched) <https://reviews.apache.org/r/66229/#comment284330> Let's `std::move()` it into the call here. src/master/http.cpp Lines 1135-1137 (patched) <https://reviews.apache.org/r/66229/#comment284325> Did authorization already happen at this point? If something isn't authorized, e.g. subscribing to one of the new roles, should this return 401? src/master/master.hpp Lines 584 (patched) <https://reviews.apache.org/r/66229/#comment284326> Does this still need to be public with the stateless validation? src/master/master.cpp Line 2346 (original), 2346 (patched) <https://reviews.apache.org/r/66229/#comment284327> Ditto here, let's keep it stateless at this point. src/master/master.cpp Lines 3407-3409 (original), 3411-3413 (patched) <https://reviews.apache.org/r/66229/#comment284329> "suppressed" src/master/master.cpp Lines 3441 (patched) <https://reviews.apache.org/r/66229/#comment284332> We're trying to update all of these to take rvalue references as part of the copy elimination work. You won't be able to move it into the .then lambda without C++14. If you add an `_updateFramework` then we could `std::move()` the call into the continuation. We should probably just do that now since it's going to come up during benchmarking. src/master/master.cpp Lines 3450 (patched) <https://reviews.apache.org/r/66229/#comment284334> Let's avoid the copy here, not sure why this code is copying.. src/master/master.cpp Lines 3464-3466 (patched) <https://reviews.apache.org/r/66229/#comment284333> Can you avoid copying the roles into the set here? This gets expensive if frameworks have e.g. 10,000 roles. src/master/master.cpp Lines 3468 (patched) <https://reviews.apache.org/r/66229/#comment284335> Ideally the two parameters could be std::moved in. Let's not worry about it here. If you want to send a follow up patch to eliminate copies in this updateFramework() overload that would be great! If not, it's going to come up during benchmarking :) src/master/metrics.hpp Lines 142 (patched) <https://reviews.apache.org/r/66229/#comment284336> I don't think we want to make up v0 message metrics for v1 calls. Can you ask greg/gaston about this? I think the plan is to try to map v0 to v1 calls instead of adding v0 style metrics for v1 calls. src/master/validation.cpp Lines 491-493 (patched) <https://reviews.apache.org/r/66229/#comment284344> The contents of this look familiar, is it copied from the re-registration path? Can you add appropriate TODOs about the inconsistency between the re-registration path and the UPDATE_FRAMEWORK path? src/master/validation.cpp Lines 495-499 (patched) <https://reviews.apache.org/r/66229/#comment284339> Is this still needed if we keep the call validation as stateless validation? src/master/validation.cpp Lines 508-509 (patched) <https://reviews.apache.org/r/66229/#comment284343> Just as an aside, this copying could get rather expensive for frameworks with e.g. 10,000 roles. Don't need to address it here, but the fix I suppose would be to do fine grained equality checking here. src/master/validation.cpp Lines 511-517 (patched) <https://reviews.apache.org/r/66229/#comment284340> I guess this function needs a significant update once it accepts the same changes to the framework info as are allowed in the re-registration path? src/master/validation.cpp Lines 535-536 (patched) <https://reviews.apache.org/r/66229/#comment284341> It's odd to log this one case, why isn't the caller logging this when logging the error? src/master/validation.cpp Line 538 (original), 606 (patched) <https://reviews.apache.org/r/66229/#comment284338> Can you send a separate patch for this or just commit it directly on master? src/master/validation.cpp Lines 721 (patched) <https://reviews.apache.org/r/66229/#comment284337> This function is just stateless validation for now, per above comments. Ideally this function would also do stateless validation of the messages within the call, but that's a cleanup to do separately. - Benjamin Mahler On May 3, 2018, 4:05 a.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66229/ > ----------------------------------------------------------- > > (Updated May 3, 2018, 4:05 a.m.) > > > Review request for mesos, Benjamin Mahler and Till Toenshoff. > > > Bugs: MESOS-7258 > https://issues.apache.org/jira/browse/MESOS-7258 > > > Repository: mesos > > > Description > ------- > > Implemented UPDATE_FRAMEWORK call. > > > Diffs > ----- > > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > ea8d54ff198a5529d61a41bcb6e5806378761091 > src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad > src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 > src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 > src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b > src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 > src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf > src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d > > > Diff: https://reviews.apache.org/r/66229/diff/12/ > > > Testing > ------- > > > Thanks, > > Kapil Arya > >
