Re: Review Request 41930: Added "TeardownFramework" to ACL protobuf.

[Adam B]

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

Ship it!


Ship It!

- Adam B


On Jan. 13, 2016, 11:51 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41930/
> ---
> 
> (Updated Jan. 13, 2016, 11:51 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4154
> https://issues.apache.org/jira/browse/MESOS-4154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added "TeardownFramework" to ACL protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 84727e66dd14be9a25705ab1141e92eee72fae50 
> 
> Diff: https://reviews.apache.org/r/41930/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41930: Added "TeardownFramework" to ACL protobuf.

[Guangya Liu]

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

(Updated 一月 14, 2016, 7:51 a.m.)


Review request for mesos and Adam B.


Bugs: MESOS-4154
https://issues.apache.org/jira/browse/MESOS-4154


Repository: mesos


Description (updated)
---

Added "TeardownFramework" to ACL protobuf.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
84727e66dd14be9a25705ab1141e92eee72fae50 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 41930: Added "TeardownFramework" to ACL protobuf.

[Guangya Liu]

> On 一月 11, 2016, 6:44 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.proto, lines 147-148
> > 
> >
> > What is the expected behavior if both are set? Error? Union?

Yes, Union.


> On 一月 11, 2016, 6:44 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.proto, line 148
> > 
> >
> > This is never used? Or is that in a subsequent patch?

subsequent patch.


> On 一月 11, 2016, 6:44 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 77
> > 
> >
> > Did you mean to iterate through `shutdown_frameworks()` or 
> > `teardown_frameworks()`? Or both?

iterate both in the dependency patch.


- Guangya


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


On 一月 6, 2016, 2:35 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41930/
> ---
> 
> (Updated 一月 6, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4154
> https://issues.apache.org/jira/browse/MESOS-4154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch included the following:
> 1) Renamed "ShutdownFramework" to "TeardownFramework".
> 2) Added teardown_frameworks ACL to protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> f61613948b7b5c5c2118f1782a0c5f8ff7e8e057 
>   include/mesos/authorizer/authorizer.proto 
> 7b729e19484d92be48bbde4dff2c833a4109936e 
>   src/authorizer/local/authorizer.hpp 
> 1563c11709c2612350354690b50ceb33d2720f98 
>   src/authorizer/local/authorizer.cpp 
> 1d135fb6906c7050a788cbac9ca2d8164ff064ef 
>   src/master/http.cpp d7afa2af989eebfc9039b3681f087ce570f601d5 
>   src/tests/master_tests.cpp 865fa4a71f4bae2a218cd2c4e10873222d1ea3c4 
>   src/tests/mesos.hpp 49a4c48e6887e6f0921d96c359746e39be10e222 
>   src/tests/mesos.cpp 082e57bc73fad02de77e16e4b34451e6c0903038 
>   src/tests/slave_recovery_tests.cpp c0e4ff75b35c9e806741aab5696771e66d2c2ea8 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/41930/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41930: Added "TeardownFramework" to ACL protobuf.

[Adam B]

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



include/mesos/authorizer/authorizer.proto (line 62)


I don't think you can safely rename a protobuf message type like this. You 
can rename a field, since it has an id to uniquely identify it, but I believe 
you need to add a new (nearly identical) message type to support the change to 
TeardownFramework. Please correct me if you've found evidence indicating 
otherwise.
Have you tried testing this change across versions, where an authorizer 
compiles against the old version but the master uses the new version? Or vice 
versa?



include/mesos/authorizer/authorizer.proto (lines 147 - 148)


What is the expected behavior if both are set? Error? Union?



include/mesos/authorizer/authorizer.proto (line 148)


This is never used? Or is that in a subsequent patch?



src/authorizer/local/authorizer.cpp (line 77)


Did you mean to iterate through `shutdown_frameworks()` or 
`teardown_frameworks()`? Or both?


- Adam B


On Jan. 6, 2016, 6:35 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41930/
> ---
> 
> (Updated Jan. 6, 2016, 6:35 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4154
> https://issues.apache.org/jira/browse/MESOS-4154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch included the following:
> 1) Renamed "ShutdownFramework" to "TeardownFramework".
> 2) Added teardown_frameworks ACL to protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> f61613948b7b5c5c2118f1782a0c5f8ff7e8e057 
>   include/mesos/authorizer/authorizer.proto 
> 7b729e19484d92be48bbde4dff2c833a4109936e 
>   src/authorizer/local/authorizer.hpp 
> 1563c11709c2612350354690b50ceb33d2720f98 
>   src/authorizer/local/authorizer.cpp 
> 1d135fb6906c7050a788cbac9ca2d8164ff064ef 
>   src/master/http.cpp d7afa2af989eebfc9039b3681f087ce570f601d5 
>   src/tests/master_tests.cpp 865fa4a71f4bae2a218cd2c4e10873222d1ea3c4 
>   src/tests/mesos.hpp 49a4c48e6887e6f0921d96c359746e39be10e222 
>   src/tests/mesos.cpp 082e57bc73fad02de77e16e4b34451e6c0903038 
>   src/tests/slave_recovery_tests.cpp c0e4ff75b35c9e806741aab5696771e66d2c2ea8 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/41930/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>