----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59861/#review177538 -----------------------------------------------------------
include/mesos/mesos.proto Lines 347-354 (patched) <https://reviews.apache.org/r/59861/#comment251174> Do you want to comment on the semantics with respect to the filtering of offers? From our discussion, you were saying that we don't send refined reservations (i.e. more than 1 level) to frameworks without the capability. It doesn't quite seem like this is the intent to create refined reservations, they could just want to consume them, or use the new format? It would be great to steer them a little more towards this "this enables the new format, which has X feature" rather than "provide this if you want to use X feature". include/mesos/mesos.proto Lines 822-824 (patched) <https://reviews.apache.org/r/59861/#comment251355> Not just launch tasks, but handle reservations, volumes, etc, so it's more a matter of offer operations? I see that this was just copy paste from HIERARCHICAL_ROLE, but would be good to be more precise here. include/mesos/mesos.proto Lines 934 (patched) <https://reviews.apache.org/r/59861/#comment251356> I don't think people are going to understand the old vs new format based on this NOTE. Could you expand on that here? E.g. In the old format (pre-XXX capability), role means X and reservation is set when X. In the new format (post-XXX capability), role is unused, and instead we set 'reservations' when a resource is reserved. Also, should we have a TODO here for marking 'role' as deprecated? include/mesos/mesos.proto Lines 962-965 (patched) <https://reviews.apache.org/r/59861/#comment251358> Per [MESOS-4997](https://issues.apache.org/jira/browse/MESOS-4997), could you add an 'UNKNOWN = 0' here? Also, with that high level format comment I mentioned, it would be easier for people to understand that we didn't use to have 'Type' when we only had the 'reservation' field. include/mesos/mesos.proto Lines 967-969 (patched) <https://reviews.apache.org/r/59861/#comment251357> It's a little tough to figure out the meaning behind this NOTE, if you had a high level description of the old and new formats (per my previous comment), you could add a reference to that here, so that the reader can understand this better. include/mesos/mesos.proto Lines 971-973 (patched) <https://reviews.apache.org/r/59861/#comment251359> Ditto here w.r.t. the NOTE. include/mesos/mesos.proto Lines 998 (patched) <https://reviews.apache.org/r/59861/#comment251360> "The stack of reservations"? include/mesos/mesos.proto Lines 1006 (patched) <https://reviews.apache.org/r/59861/#comment251361> End with a period like the other ones you added? src/slave/constants.cpp Lines 41 (patched) <https://reviews.apache.org/r/59861/#comment251363> Can we introduce this capability *after* the agent can handle it? We can test in the interim by injecting the capability during (re-)registration. src/tests/master_tests.cpp Lines 4287-4288 (original), 4287-4288 (patched) <https://reviews.apache.org/r/59861/#comment251362> Comment needs updating, ditto below. But, see my above comment. - Benjamin Mahler On June 7, 2017, 2:54 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59861/ > ----------------------------------------------------------- > > (Updated June 7, 2017, 2:54 a.m.) > > > Review request for mesos, Benjamin Mahler and Neil Conway. > > > Bugs: MESOS-7575 > https://issues.apache.org/jira/browse/MESOS-7575 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc > include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 > src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 > src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d > src/slave/constants.cpp 0fbcab8f1cee4183dfd4c25984cd27f8baabda44 > src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 > src/tests/slave_tests.cpp b5141d7013acdd6e236606ef3d9b1953b14d373a > > > Diff: https://reviews.apache.org/r/59861/diff/1/ > > > Testing > ------- > > Updated tests + `make check` > > > Thanks, > > Michael Park > >
