You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/11/05 22:43:35 UTC
Review Request 39985: [1/5] Introduced ACL protobuf definitions for
dynamic reservation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/
-----------------------------------------------------------
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Introduced ACL protobuf definitions for dynamic reservation.
Diffs
-----
include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
Diff: https://reviews.apache.org/r/39985/diff/
Testing
-------
This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
Thanks,
Greg Mann
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 13, 2015, 9:59 p.m., Jie Yu wrote:
> > include/mesos/authorizer/authorizer.proto, line 75
> > <https://reviews.apache.org/r/39985/diff/3/?file=1120023#file1120023line75>
> >
> > Can we call it ReserveResources to match other ACLs? (also, in case we have other things that we want to reserve in the future).
Thanks Jie, good call! I'm currently updating the other patches in the chain to accomodate this change.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review106509
-----------------------------------------------------------
On Nov. 13, 2015, 11:11 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2015, 11:11 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
> Note: documentation for these changes is included in https://reviews.apache.org/r/40271/
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review106509
-----------------------------------------------------------
Ship it!
include/mesos/authorizer/authorizer.proto (line 73)
<https://reviews.apache.org/r/39985/#comment165216>
Can we call it ReserveResources to match other ACLs? (also, in case we have other things that we want to reserve in the future).
- Jie Yu
On Nov. 13, 2015, 4:34 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2015, 4:34 a.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
> Note: documentation for these changes is included in https://reviews.apache.org/r/40271/
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review108483
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Nov. 30, 2015, 8:32 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2015, 8:32 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 8e72003f405770f00c5d87f318a9e1a8ed7430ee
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
> Note: documentation for these changes is included in https://reviews.apache.org/r/40271/
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/
-----------------------------------------------------------
(Updated Nov. 30, 2015, 8:32 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Rebase.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Introduced ACL protobuf definitions for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37002/
Diffs (updated)
-----
include/mesos/authorizer/authorizer.proto 8e72003f405770f00c5d87f318a9e1a8ed7430ee
Diff: https://reviews.apache.org/r/39985/diff/
Testing
-------
This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
Note: documentation for these changes is included in https://reviews.apache.org/r/40271/
Thanks,
Greg Mann
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/
-----------------------------------------------------------
(Updated Nov. 13, 2015, 11:11 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Changed ACL names.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Introduced ACL protobuf definitions for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37002/
Diffs (updated)
-----
include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
Diff: https://reviews.apache.org/r/39985/diff/
Testing
-------
This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
Note: documentation for these changes is included in https://reviews.apache.org/r/40271/
Thanks,
Greg Mann
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/
-----------------------------------------------------------
(Updated Nov. 13, 2015, 4:34 a.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Added link to review with documentation additions.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Introduced ACL protobuf definitions for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37002/
Diffs
-----
include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
Diff: https://reviews.apache.org/r/39985/diff/
Testing (updated)
-------
This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
Note: documentation for these changes is included in https://reviews.apache.org/r/40271/
Thanks,
Greg Mann
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 5:25 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Changes
-------
Rebase.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Introduced ACL protobuf definitions for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37002/
Diffs (updated)
-----
include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
Diff: https://reviews.apache.org/r/39985/diff/
Testing
-------
This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
Thanks,
Greg Mann
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105596
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On 十一月 6, 2015, 11:52 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated 十一月 6, 2015, 11:52 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/
-----------------------------------------------------------
(Updated Nov. 6, 2015, 11:52 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description
-------
Introduced ACL protobuf definitions for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37002/
Diffs (updated)
-----
include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
Diff: https://reviews.apache.org/r/39985/diff/
Testing
-------
This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
Thanks,
Greg Mann
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 6, 2015, 7:22 a.m., Guangya Liu wrote:
> > include/mesos/authorizer/authorizer.proto, line 28
> > <https://reviews.apache.org/r/39985/diff/1/?file=1116935#file1116935line28>
> >
> > We have some discussions want to reduce the comments to 70 chars per line.
>
> Adam B wrote:
> We actually recently changed the style guide to no longer require comments to wrap at 70 chars.
> Now comments can/should wrap at 80 chars like everything else. We will not do a sweeping change.
>
> Guangya Liu wrote:
> I see that we are still under discussion for this in mail list, seems @bmahler has some different comments, please search "Mesos Style Guideline Adjustments?" in dev list for detail.
>
> Just cite the comments from bmahler from dev list:
> This has come up in a couple of reviews, seems like we should add some soft
> guidelines around how to format comments for readability.
>
> In particular, the reason that we wrapped at 70 in the past was for
> readability, so it would be great to continue doing so as a soft stylistic
> rule. The other thing we've been doing for readability is reducing
> "jaggedness" (variability in line lengths).
>
> It would be great to establish these as soft rules and encourage new
> contributors / committers to follow them. Compare these two comments in
> Master::updateTask. The first one wraps at 70 and reduces jagedness, the
> second wraps at 80 and is more jagged:
>
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6057
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6072
>
> I can provide more examples to help clarify. If no one objects, I'll follow
> up with an update to the style guide. Thoughts appreciated!
>
> Guangya Liu wrote:
> You can also refer here: http://search-hadoop.com/m/0Vlr6MlNkc1FGzCj2/Mesos+Style+Guideline+Adjustments%25E2%2580%258F+Ben+Mahler+This+has+come+up+in+a+couple+of+reviews%252C+seems+like+we+should+add+some+soft&subj=Re+Mesos+Style+Guideline+Adjustments
>
> This is still under discussion.
>
> Adam B wrote:
> Fair enough, but if the goal is to reduce jaggedness, that's yet another argument for keeping this comment all on one line, even past 70 chars.
> (Thanks for the discussion reference. I knew that we'd recently removed the 70 char restriction, but wasn't abreast of the ongoing discussion.)
>
> Guangya Liu wrote:
> Thanks Adam! @Greg, you can just drop this comment if we did not reach an agreement on this before submit your patch. ;-)
Yea, I think that the patch should adhere to the style guide as it currently exists. If the style guide is changed before these patches land, I can change the comment wrapping. Otherwise, I'll leave as-is.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105376
-----------------------------------------------------------
On Nov. 6, 2015, 11:52 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 11:52 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Adam B <ad...@mesosphere.io>.
> On Nov. 5, 2015, 11:22 p.m., Guangya Liu wrote:
> > include/mesos/authorizer/authorizer.proto, line 28
> > <https://reviews.apache.org/r/39985/diff/1/?file=1116935#file1116935line28>
> >
> > We have some discussions want to reduce the comments to 70 chars per line.
We actually recently changed the style guide to no longer require comments to wrap at 70 chars.
Now comments can/should wrap at 80 chars like everything else. We will not do a sweeping change.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105376
-----------------------------------------------------------
On Nov. 5, 2015, 1:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 1:50 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Guangya Liu <gy...@gmail.com>.
> On 十一月 6, 2015, 7:22 a.m., Guangya Liu wrote:
> > include/mesos/authorizer/authorizer.proto, line 28
> > <https://reviews.apache.org/r/39985/diff/1/?file=1116935#file1116935line28>
> >
> > We have some discussions want to reduce the comments to 70 chars per line.
>
> Adam B wrote:
> We actually recently changed the style guide to no longer require comments to wrap at 70 chars.
> Now comments can/should wrap at 80 chars like everything else. We will not do a sweeping change.
>
> Guangya Liu wrote:
> I see that we are still under discussion for this in mail list, seems @bmahler has some different comments, please search "Mesos Style Guideline Adjustments?" in dev list for detail.
>
> Just cite the comments from bmahler from dev list:
> This has come up in a couple of reviews, seems like we should add some soft
> guidelines around how to format comments for readability.
>
> In particular, the reason that we wrapped at 70 in the past was for
> readability, so it would be great to continue doing so as a soft stylistic
> rule. The other thing we've been doing for readability is reducing
> "jaggedness" (variability in line lengths).
>
> It would be great to establish these as soft rules and encourage new
> contributors / committers to follow them. Compare these two comments in
> Master::updateTask. The first one wraps at 70 and reduces jagedness, the
> second wraps at 80 and is more jagged:
>
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6057
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6072
>
> I can provide more examples to help clarify. If no one objects, I'll follow
> up with an update to the style guide. Thoughts appreciated!
>
> Guangya Liu wrote:
> You can also refer here: http://search-hadoop.com/m/0Vlr6MlNkc1FGzCj2/Mesos+Style+Guideline+Adjustments%25E2%2580%258F+Ben+Mahler+This+has+come+up+in+a+couple+of+reviews%252C+seems+like+we+should+add+some+soft&subj=Re+Mesos+Style+Guideline+Adjustments
>
> This is still under discussion.
>
> Adam B wrote:
> Fair enough, but if the goal is to reduce jaggedness, that's yet another argument for keeping this comment all on one line, even past 70 chars.
> (Thanks for the discussion reference. I knew that we'd recently removed the 70 char restriction, but wasn't abreast of the ongoing discussion.)
Thanks Adam! @Greg, you can just drop this comment if we did not reach an agreement on this before submit your patch. ;-)
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105376
-----------------------------------------------------------
On 十一月 5, 2015, 9:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated 十一月 5, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Guangya Liu <gy...@gmail.com>.
> On 十一月 6, 2015, 7:22 a.m., Guangya Liu wrote:
> > include/mesos/authorizer/authorizer.proto, line 28
> > <https://reviews.apache.org/r/39985/diff/1/?file=1116935#file1116935line28>
> >
> > We have some discussions want to reduce the comments to 70 chars per line.
>
> Adam B wrote:
> We actually recently changed the style guide to no longer require comments to wrap at 70 chars.
> Now comments can/should wrap at 80 chars like everything else. We will not do a sweeping change.
I see that we are still under discussion for this in mail list, seems @bmahler has some different comments, please search "Mesos Style Guideline Adjustments?" in dev list for detail.
Just cite the comments from bmahler from dev list:
This has come up in a couple of reviews, seems like we should add some soft
guidelines around how to format comments for readability.
In particular, the reason that we wrapped at 70 in the past was for
readability, so it would be great to continue doing so as a soft stylistic
rule. The other thing we've been doing for readability is reducing
"jaggedness" (variability in line lengths).
It would be great to establish these as soft rules and encourage new
contributors / committers to follow them. Compare these two comments in
Master::updateTask. The first one wraps at 70 and reduces jagedness, the
second wraps at 80 and is more jagged:
https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6057
https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6072
I can provide more examples to help clarify. If no one objects, I'll follow
up with an update to the style guide. Thoughts appreciated!
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105376
-----------------------------------------------------------
On 十一月 5, 2015, 9:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated 十一月 5, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Guangya Liu <gy...@gmail.com>.
> On 十一月 6, 2015, 7:22 a.m., Guangya Liu wrote:
> > include/mesos/authorizer/authorizer.proto, line 28
> > <https://reviews.apache.org/r/39985/diff/1/?file=1116935#file1116935line28>
> >
> > We have some discussions want to reduce the comments to 70 chars per line.
>
> Adam B wrote:
> We actually recently changed the style guide to no longer require comments to wrap at 70 chars.
> Now comments can/should wrap at 80 chars like everything else. We will not do a sweeping change.
>
> Guangya Liu wrote:
> I see that we are still under discussion for this in mail list, seems @bmahler has some different comments, please search "Mesos Style Guideline Adjustments?" in dev list for detail.
>
> Just cite the comments from bmahler from dev list:
> This has come up in a couple of reviews, seems like we should add some soft
> guidelines around how to format comments for readability.
>
> In particular, the reason that we wrapped at 70 in the past was for
> readability, so it would be great to continue doing so as a soft stylistic
> rule. The other thing we've been doing for readability is reducing
> "jaggedness" (variability in line lengths).
>
> It would be great to establish these as soft rules and encourage new
> contributors / committers to follow them. Compare these two comments in
> Master::updateTask. The first one wraps at 70 and reduces jagedness, the
> second wraps at 80 and is more jagged:
>
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6057
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6072
>
> I can provide more examples to help clarify. If no one objects, I'll follow
> up with an update to the style guide. Thoughts appreciated!
You can also refer here: http://search-hadoop.com/m/0Vlr6MlNkc1FGzCj2/Mesos+Style+Guideline+Adjustments%25E2%2580%258F+Ben+Mahler+This+has+come+up+in+a+couple+of+reviews%252C+seems+like+we+should+add+some+soft&subj=Re+Mesos+Style+Guideline+Adjustments
This is still under discussion.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105376
-----------------------------------------------------------
On 十一月 5, 2015, 9:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated 十一月 5, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Adam B <ad...@mesosphere.io>.
> On Nov. 5, 2015, 11:22 p.m., Guangya Liu wrote:
> > include/mesos/authorizer/authorizer.proto, line 28
> > <https://reviews.apache.org/r/39985/diff/1/?file=1116935#file1116935line28>
> >
> > We have some discussions want to reduce the comments to 70 chars per line.
>
> Adam B wrote:
> We actually recently changed the style guide to no longer require comments to wrap at 70 chars.
> Now comments can/should wrap at 80 chars like everything else. We will not do a sweeping change.
>
> Guangya Liu wrote:
> I see that we are still under discussion for this in mail list, seems @bmahler has some different comments, please search "Mesos Style Guideline Adjustments?" in dev list for detail.
>
> Just cite the comments from bmahler from dev list:
> This has come up in a couple of reviews, seems like we should add some soft
> guidelines around how to format comments for readability.
>
> In particular, the reason that we wrapped at 70 in the past was for
> readability, so it would be great to continue doing so as a soft stylistic
> rule. The other thing we've been doing for readability is reducing
> "jaggedness" (variability in line lengths).
>
> It would be great to establish these as soft rules and encourage new
> contributors / committers to follow them. Compare these two comments in
> Master::updateTask. The first one wraps at 70 and reduces jagedness, the
> second wraps at 80 and is more jagged:
>
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6057
> https://github.com/apache/mesos/blob/0.25.0/src/master/master.cpp#L6072
>
> I can provide more examples to help clarify. If no one objects, I'll follow
> up with an update to the style guide. Thoughts appreciated!
>
> Guangya Liu wrote:
> You can also refer here: http://search-hadoop.com/m/0Vlr6MlNkc1FGzCj2/Mesos+Style+Guideline+Adjustments%25E2%2580%258F+Ben+Mahler+This+has+come+up+in+a+couple+of+reviews%252C+seems+like+we+should+add+some+soft&subj=Re+Mesos+Style+Guideline+Adjustments
>
> This is still under discussion.
Fair enough, but if the goal is to reduce jaggedness, that's yet another argument for keeping this comment all on one line, even past 70 chars.
(Thanks for the discussion reference. I knew that we'd recently removed the 70 char restriction, but wasn't abreast of the ongoing discussion.)
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105376
-----------------------------------------------------------
On Nov. 5, 2015, 1:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 1:50 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/#review105376
-----------------------------------------------------------
include/mesos/authorizer/authorizer.proto (line 28)
<https://reviews.apache.org/r/39985/#comment163951>
We have some discussions want to reduce the comments to 70 chars per line.
- Guangya Liu
On 十一月 5, 2015, 9:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> -----------------------------------------------------------
>
> (Updated 十一月 5, 2015, 9:50 p.m.)
>
>
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
>
>
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
>
>
> Diffs
> -----
>
> include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
>
> Diff: https://reviews.apache.org/r/39985/diff/
>
>
> Testing
> -------
>
> This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions
for dynamic reservation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39985/
-----------------------------------------------------------
(Updated Nov. 5, 2015, 9:50 p.m.)
Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
Bugs: MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3062
Repository: mesos
Description (updated)
-------
Introduced ACL protobuf definitions for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37002/
Diffs
-----
include/mesos/authorizer/authorizer.proto 86bbb45f9d91b4098a262e3e50a793f3bb39497e
Diff: https://reviews.apache.org/r/39985/diff/
Testing
-------
This is the first in a chain of 5 patches. `make check` was used to test at the end of the chain.
Thanks,
Greg Mann