Re: Review Request 45317: Change Call and Event Type enums in scheduler.proto optional.

[Anand Mazumdar]

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