----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66050/#review201887 -----------------------------------------------------------
Fix it, then Ship it! src/master/master.cpp Lines 4905-4923 (patched) <https://reviews.apache.org/r/66050/#comment283502> I just noticed that we created some inconsistency for the `validation::operation::validate()` functions when we introduced the new non-speculative operations. For the conventional operations, we check the agent capabilities in the validate function; for the new operations, we do the check here in place. How about moving the capability checks into the validate functions and adjust the error message as follows ``` "Volume " + stringify(volume) + " cannot be grown on an agent without RESOURCE_PROVIDER capability" ``` so we don't need multiple `drop` here and make the `_accept` function cleaner? I could follow up with a patch to clean up the inconsistency for `CREATE_VOLUME`, `CREATE_BLOCK`, `DESTROY_VOLUME` and `DESTROY_BLOCK`. src/master/master.cpp Lines 4975-4993 (patched) <https://reviews.apache.org/r/66050/#comment283503> How about moving the capability checks into the validate functions and adjust the error message as follows ``` "Volume " + stringify(volume) + " cannot be shrunk on an agent without RESOURCE_PROVIDER capability" ``` so we don't need multiple `drop` here and make the `_accept` function cleaner? src/master/validation.cpp Lines 2332 (patched) <https://reviews.apache.org/r/66050/#comment283509> Can you add tests for this function in `src/tests/master_validation_tests.cpp`? To unblock this patch, it can be done in a follow-up one. src/master/validation.cpp Lines 2354 (patched) <https://reviews.apache.org/r/66050/#comment283504> I see below you use `size of 'ShrinkVolume.volume'`, which reads more straight-forward, so let's use `"The size of 'GrowVolume.addition' must ..."` here as well. src/master/validation.cpp Lines 2388 (patched) <https://reviews.apache.org/r/66050/#comment283505> `the '...' and '...' fields` src/master/validation.cpp Lines 2396 (patched) <https://reviews.apache.org/r/66050/#comment283510> Can you add tests for this function in `src/tests/master_validation_tests.cpp`? To unblock this patch, it can be done in a follow-up one. src/master/validation.cpp Lines 2416 (patched) <https://reviews.apache.org/r/66050/#comment283506> `smaller than the size ` src/master/validation.cpp Lines 2428 (patched) <https://reviews.apache.org/r/66050/#comment283508> Suggestion: `"Shrinking a volume on a MOUNT disk ..."`. - Chun-Hung Hsiao On April 24, 2018, 9:16 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66050/ > ----------------------------------------------------------- > > (Updated April 24, 2018, 9:16 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann. > > > Bugs: MESOS-4965 > https://issues.apache.org/jira/browse/MESOS-4965 > > > Repository: mesos > > > Description > ------- > > The new offer operations are implemented as speculative operations, but > we will use validation to make them non-speculative on API level so that > we can transition later without a breaking change. > > > Diffs > ----- > > src/common/protobuf_utils.cpp 78bffd8595f0e9f34e981548d8136ff94160573b > src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 > src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 > src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf > src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d > > > Diff: https://reviews.apache.org/r/66050/diff/13/ > > > Testing > ------- > > > Thanks, > > Zhitao Li > >
