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

[Ben Mahler]

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




src/tests/mesos.hpp (lines 932 - 935)


Yikes, this case isn't actually unreachable!

Per MESOS-2664 and MESOS-3754, please avoid 'default' in favor of an 
explicit 'case UNKNOWN' so that the compiler helps us catch all switches when 
we introduce a new enum value.


- Ben Mahler


On March 25, 2016, 8:55 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45317/
> ---
> 
> (Updated March 25, 2016, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar 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
> -
> 
>   CHANGELOG fef0cbfc3ae282707569429d38e52c19d4eb9aba 
>   include/mesos/scheduler/scheduler.proto 
> 0049e1383f50574c3dad6a29b91811001694e82c 
>   include/mesos/v1/scheduler/scheduler.proto 
> 09fafedd06837f2307fedc6fa0e7b4460b21e5b0 
>   src/master/validation.cpp 701a5c4b279f319dde15bd8f2e97b5fd8608e578 
>   src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 
> 
> Diff: https://reviews.apache.org/r/45317/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

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

[Yong Tang]

> On March 25, 2016, 8:20 p.m., Vinod Kone wrote:
> > LGTM.
> > 
> > Can you update the CHANGELOG and call out this change in "Additonal API 
> > Changes" section for 0.29.0?

Thanks Vinod. Just updated the CHANGELOG.


- Yong


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


On March 25, 2016, 8:55 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45317/
> ---
> 
> (Updated March 25, 2016, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar 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
> -
> 
>   CHANGELOG fef0cbfc3ae282707569429d38e52c19d4eb9aba 
>   include/mesos/scheduler/scheduler.proto 
> 0049e1383f50574c3dad6a29b91811001694e82c 
>   include/mesos/v1/scheduler/scheduler.proto 
> 09fafedd06837f2307fedc6fa0e7b4460b21e5b0 
>   src/master/validation.cpp 701a5c4b279f319dde15bd8f2e97b5fd8608e578 
>   src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 
> 
> Diff: https://reviews.apache.org/r/45317/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

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

[Yong Tang]

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

(Updated March 25, 2016, 8:55 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Updated the CHANGELOG to reference this change for 0.29.0.


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 (updated)
-

  CHANGELOG fef0cbfc3ae282707569429d38e52c19d4eb9aba 
  include/mesos/scheduler/scheduler.proto 
0049e1383f50574c3dad6a29b91811001694e82c 
  include/mesos/v1/scheduler/scheduler.proto 
09fafedd06837f2307fedc6fa0e7b4460b21e5b0 
  src/master/validation.cpp 701a5c4b279f319dde15bd8f2e97b5fd8608e578 
  src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 

Diff: https://reviews.apache.org/r/45317/diff/


Testing
---

make check (Ubuntu 14.04)


Thanks,

Yong Tang

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

[Vinod Kone]

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



LGTM.

Can you update the CHANGELOG and call out this change in "Additonal API 
Changes" section for 0.29.0?

- Vinod Kone


On March 25, 2016, 5:56 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45317/
> ---
> 
> (Updated March 25, 2016, 5:56 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar 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/master/validation.cpp 701a5c4b279f319dde15bd8f2e97b5fd8608e578 
>   src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 
> 
> Diff: https://reviews.apache.org/r/45317/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

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/#review125462
---


Ship it!




LGTM

- Anand Mazumdar


On March 25, 2016, 5:56 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45317/
> ---
> 
> (Updated March 25, 2016, 5:56 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar 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/master/validation.cpp 701a5c4b279f319dde15bd8f2e97b5fd8608e578 
>   src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 
> 
> Diff: https://reviews.apache.org/r/45317/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

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

[Yong Tang]

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

(Updated March 25, 2016, 5:56 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Add Anand to the reviewers list.


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 (updated)
-

  include/mesos/scheduler/scheduler.proto 
0049e1383f50574c3dad6a29b91811001694e82c 
  include/mesos/v1/scheduler/scheduler.proto 
09fafedd06837f2307fedc6fa0e7b4460b21e5b0 
  src/master/validation.cpp 701a5c4b279f319dde15bd8f2e97b5fd8608e578 
  src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 

Diff: https://reviews.apache.org/r/45317/diff/


Testing
---

make check (Ubuntu 14.04)


Thanks,

Yong Tang

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)


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)


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)


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)


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)


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

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45317]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


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