mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <mazumdar.an...@gmail.com>
Subject Re: Review Request 45317: Change Call and Event Type enums in scheduler.proto optional.
Date Fri, 25 Mar 2016 17:02:28 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45317/#review125427
-----------------------------------------------------------



Thanks for working on this. Added some comments.

Also, one more additional change would be needed:

Since now that the `Type` field is optional we would like to add validation on master to ignore
the request if the `Type` field does not exist.

Here: https://github.com/apache/mesos/blob/master/src/master/validation.cpp#L61

Let's add a check like this:

```cpp
if (!call.has_type()) {
  return Error("Expecting 'type' to be present");
}
```


include/mesos/v1/scheduler/scheduler.proto (lines 36 - 37)
<https://reviews.apache.org/r/45317/#comment188219>

    Let's add an identical comment to the one that exists in `mesos.proto` with a link to
the epic. Also, In the near future it would be good to have this comment once per file and
not being redundant.
    
    ```cpp
    // This must be the first enum value in this list, to ensure that if 'type' is not set,
the default value is UNKNOWN. This enables enum values to be added in a backwards-compatible
way. See: MESOS-4997.
    ```



include/mesos/v1/scheduler/scheduler.proto (line 140)
<https://reviews.apache.org/r/45317/#comment188220>

    Let's modify this line to make it identical to `mesos.proto`:
    
    ```cpp
    // Enum fields should be optional, see: MESOS-4997.
    ```



include/mesos/v1/scheduler/scheduler.proto (lines 163 - 164)
<https://reviews.apache.org/r/45317/#comment188221>

    Let's remove this redundant comment since we already added it for the `Event` type.
    
    Just this comment should suffice:
    ```cpp
    // See comments above on `Event::Type` for more details on this enum value.
    ```



include/mesos/v1/scheduler/scheduler.proto (line 314)
<https://reviews.apache.org/r/45317/#comment188222>

    Let's modify this to:
    
    ```cpp
    // See comments on `Event::Type` above on the reasoning behind this field being optional.
    ```



src/tests/mesos.hpp (lines 909 - 910)
<https://reviews.apache.org/r/45317/#comment188224>

    I would like to see much more then a `break` since receiving an `UNKNOWN` from the master
means something _bad_ has happened.
    
    Let's add a `default` case to this `switch` i.e. something like this after L934.
    
    ```cpp
    default:
      UNREACHABLE();
    ```


- Anand Mazumdar


On March 24, 2016, 9:18 p.m., Yong Tang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45317/
> -----------------------------------------------------------
> 
> (Updated March 24, 2016, 9:18 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5014
>     https://issues.apache.org/jira/browse/MESOS-5014
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This fix changes Call and Event Type enums in scheduler.proto
> optional for the purpose of backwards compatibility. (MESOS-5014)
> 
> 
> Diffs
> -----
> 
>   include/mesos/scheduler/scheduler.proto 0049e1383f50574c3dad6a29b91811001694e82c 
>   include/mesos/v1/scheduler/scheduler.proto 09fafedd06837f2307fedc6fa0e7b4460b21e5b0

>   src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 
> 
> Diff: https://reviews.apache.org/r/45317/diff/
> 
> 
> Testing
> -------
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>


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