mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 59861: Added protobuf changes for Reservation Refinement.
Date Mon, 12 Jun 2017 21:24:31 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message