Re: Review Request 41444: Cleaned up Authorizer interface.

2016-01-04 Thread Adam B 


> On Dec. 21, 2015, 3:20 p.m., Greg Mann wrote:
> > include/mesos/authorizer/authorizer.hpp, lines 129-131
> > 
> >
> > Though I know what you're saying here, this one sounds a bit confusing 
> > to me. I wonder if something more concise like "shut down a framework 
> > registered by another framework principal." might be clearer?
> 
> Alexander Rukletsov wrote:
> I reworded it a bit, but not exactly how you proposed. Mind taking a 
> second look?

FTFY


- Adam


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


On Dec. 24, 2015, 7:32 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 24, 2015, 7:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2016-01-04 Thread Adam B 

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

Ship it!


Looks good enough to me. I'll fix these little things and commit this now.


include/mesos/authorizer/authorizer.hpp (lines 63 - 64)


We can remove the "Please check..." sentence, since it repeats what's 
already in the class comment.



include/mesos/authorizer/authorizer.hpp (line 72)


s/funcion/function/



include/mesos/authorizer/authorizer.hpp (lines 112 - 113)


"to verify the given principal can shutdown a framework originally 
registered by a (potentially different) framework principal."



include/mesos/authorizer/authorizer.hpp (lines 128 - 129)


s/reserve the types of resources contained in the request/reserve the 
resources/


- Adam B


On Dec. 24, 2015, 7:32 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 24, 2015, 7:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-24 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41444]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 24, 2015, 3:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 24, 2015, 3:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-24 Thread Adam B 


> On Dec. 18, 2015, 12:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 144
> > 
> >
> > s/reserve particular resources/reserve resources/ since the only values 
> > currently allowed for `resources` are ANY or NONE.
> 
> Alexander Rukletsov wrote:
> I'd say, that's an implementation detail of `LocalAuthorizer`; AFAIK, we 
> do not enforce it anywhere. I'm fine with leaving a `NOTE` though. What do 
> you think?

That's fine. Dropping these issues.


- Adam


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


On Dec. 22, 2015, 5:53 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 22, 2015, 5:53 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-24 Thread Adam B 

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

Ship it!


Awesome! Few minor tweaks and we can ship it!


include/mesos/authorizer/authorizer.hpp (line 42)


"checked"? You mean, "the request could not be completed"? or "made" or 
"authorized"?



include/mesos/authorizer/authorizer.hpp (line 45)


"streamlining"? You mean "bundling" or "collecting" or just "allows 
multiple values"?



include/mesos/authorizer/authorizer.hpp (line 47)


s/we do not use it in Mesos code/Mesos code currently does not authorize 
multiple entities in a single call/



include/mesos/authorizer/authorizer.hpp (line 115)


where did `framework_principal` come from? YOu only talk about "the 
framework principal" prior to this. Don't introduce a new special term 
unnecessarily.



include/mesos/authorizer/authorizer.hpp (line 181)


s/request/set/?


- Adam B


On Dec. 22, 2015, 5:53 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 22, 2015, 5:53 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-24 Thread Alexander Rukletsov 


> On Dec. 24, 2015, 10:47 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 42
> > 
> >
> > "checked"? You mean, "the request could not be completed"? or "made" or 
> > "authorized"?

I like "completed".


> On Dec. 24, 2015, 10:47 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 45
> > 
> >
> > "streamlining"? You mean "bundling" or "collecting" or just "allows 
> > multiple values"?

Let's go for "bundling".


> On Dec. 24, 2015, 10:47 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 47
> > 
> >
> > s/we do not use it in Mesos code/Mesos code currently does not 
> > authorize multiple entities in a single call/

I'd say it's a bit misleading. Multiple entities are not allowed, but multiple 
values in a single entity are. How about "Mesos code currently does not 
authorize multiple principals in a single call"?


- Alexander


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


On Dec. 22, 2015, 1:53 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 22, 2015, 1:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-24 Thread Alexander Rukletsov 

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

(Updated Dec. 24, 2015, 3:32 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, and 
Till Toenshoff.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Extract a repetitive part of the function comments into a class comment. Added 
backticks, quotes when necessary, formatted comments to avoid jaggedness.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
  src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-24 Thread Alexander Rukletsov 


> On Dec. 24, 2015, 10:47 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 133
> > 
> >
> > where did `framework_principal` come from? YOu only talk about "the 
> > framework principal" prior to this. Don't introduce a new special term 
> > unnecessarily.

It references the `framework_principals` field in `ACL::ShutdownFramework`. But 
I agree that we should avoid it for clarity.


- Alexander


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


On Dec. 22, 2015, 1:53 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 22, 2015, 1:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-22 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41444]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 22, 2015, 1:53 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 22, 2015, 1:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-22 Thread Alexander Rukletsov 


> On Dec. 21, 2015, 11:20 p.m., Greg Mann wrote:
> > include/mesos/authorizer/authorizer.hpp, lines 129-131
> > 
> >
> > Though I know what you're saying here, this one sounds a bit confusing 
> > to me. I wonder if something more concise like "shut down a framework 
> > registered by another framework principal." might be clearer?

I reworded it a bit, but not exactly how you proposed. Mind taking a second 
look?


- Alexander


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


On Dec. 21, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 21, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-22 Thread Alexander Rukletsov 

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

(Updated Dec. 22, 2015, 1:53 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, and 
Till Toenshoff.


Repository: mesos


Description
---

Extract a repetitive part of the function comments into a class comment. Added 
backticks, quotes when necessary, formatted comments to avoid jaggedness.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
  src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-21 Thread Greg Mann 


> On Dec. 21, 2015, 11:20 p.m., Greg Mann wrote:
> > Thanks, this is a big improvement!

Just a few small nits below:


- Greg


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


On Dec. 21, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 21, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-21 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41444]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 21, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 21, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-21 Thread Greg Mann 

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


Thanks, this is a big improvement!


include/mesos/authorizer/authorizer.hpp (line 35)


s/authorization for/authorization of/



include/mesos/authorizer/authorizer.hpp (lines 73 - 74)


s/module only/module-only/

s/module specific/module-specific/



include/mesos/authorizer/authorizer.hpp (lines 111 - 113)


Though I know what you're saying here, this one sounds a bit confusing to 
me. I wonder if something more concise like "shut down a framework registered 
by another framework principal." might be clearer?



include/mesos/authorizer/authorizer.hpp (line 182)


s/quota with the/quota for the/


- Greg Mann


On Dec. 21, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 21, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-21 Thread Alexander Rukletsov 

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

(Updated Dec. 21, 2015, 4:04 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, and 
Till Toenshoff.


Changes
---

More comments; s/is allowed to/can.


Repository: mesos


Description
---

Extract a repetitive part of the function comments into a class comment. Added 
backticks, quotes when necessary, formatted comments to avoid jaggedness.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
  src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-21 Thread Alexander Rukletsov 


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 66
> > 
> >
> > s/on/of/

killed the entire phrase.


- Alexander


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


On Dec. 21, 2015, 3:42 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 21, 2015, 3:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-21 Thread Alexander Rukletsov 


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > Thank you for cleaning this up. It looked like an overwhelming amount of 
> > documentation for what is not really that complex of an API. It still looks 
> > a bit verbose/repetitive, so I've made some suggestions of what else to cut 
> > out.
> > I guess we're still waiting on the ACLs for create/remove persistent 
> > volumes, in MESOS-4179

... and set/remove quota, which are all committed now, except remove quota.


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, lines 64-65
> > 
> >
> > How formal. I would think you could get away with
> > s/the "authorizer.proto" file/"authorizor.proto"/
> > s|the "docs/authorization.md"|"docs/authorization.md"|

Not sure what the second substitution means, but being inspired by the first 
sentence ("How formal"), I killed most of the comment : ).


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 75
> > 
> >
> > What is "it"? Are we removing the initialize function, the acls 
> > parameter, or what?
> > 
> > This seems very related to the first paragraph "Only relevant..." which 
> > should not be the first paragraph in the doxygen, since it is in no way a 
> > summary of the method.

Good point! I moved the first paragraph here and refactored the comment.


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, lines 114-116
> > 
> >
> > You can probably shorten this here and everywhere by just saying "A 
> > failed future indicates a problem processing the request; the request can 
> > be retried."

Till suggested the following in [one of the 
reviews](https://reviews.apache.org/r/40346/): "A failed future however 
indicates a problem processing the request and the request can be retried." 
Almost identical to your proposal.


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 126
> > 
> >
> > s/RunTask/ShutdownFramework/

Good catch!


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 144
> > 
> >
> > s/reserve particular resources/reserve resources/ since the only values 
> > currently allowed for `resources` are ANY or NONE.

I'd say, that's an implementation detail of `LocalAuthorizer`; AFAIK, we do not 
enforce it anywhere. I'm fine with leaving a `NOTE` though. What do you think?


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, line 153
> > 
> >
> > s/reserve one or more types of resources/reserve resources/

See above.


> On Dec. 18, 2015, 8:42 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.hpp, lines 155-156
> > 
> >
> > s/reserve the types of resources contained in the request/reserve 
> > resources/

See above.


- Alexander


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


On Dec. 21, 2015, 3:42 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 21, 2015, 3:42 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-21 Thread Alexander Rukletsov 

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

(Updated Dec. 21, 2015, 3:42 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, and 
Till Toenshoff.


Repository: mesos


Description
---

Extract a repetitive part of the function comments into a class comment. Added 
backticks, quotes when necessary, formatted comments to avoid jaggedness.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
19f6e1a2d025bf6ff07f515b10d41e8a48d7d0b4 
  src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-18 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [41444]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 16, 2015, 1:03 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 16, 2015, 1:03 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41444: Cleaned up Authorizer interface.

2015-12-18 Thread Adam B 

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


Thank you for cleaning this up. It looked like an overwhelming amount of 
documentation for what is not really that complex of an API. It still looks a 
bit verbose/repetitive, so I've made some suggestions of what else to cut out.
I guess we're still waiting on the ACLs for create/remove persistent volumes, 
in MESOS-4179


include/mesos/authorizer/authorizer.hpp (line 40)


s/logic/interpretation/? Or "meaning"?
Or s/has the same logic/can be interpreted the same/



include/mesos/authorizer/authorizer.hpp (lines 64 - 65)


How formal. I would think you could get away with
s/the "authorizer.proto" file/"authorizor.proto"/
s|the "docs/authorization.md"|"docs/authorization.md"|



include/mesos/authorizer/authorizer.hpp (line 66)


s/on/of/



include/mesos/authorizer/authorizer.hpp (line 69)


s/autorizer/authorizer/
s/on/of/



include/mesos/authorizer/authorizer.hpp (line 75)


What is "it"? Are we removing the initialize function, the acls parameter, 
or what?

This seems very related to the first paragraph "Only relevant..." which 
should not be the first paragraph in the doxygen, since it is in no way a 
summary of the method.



include/mesos/authorizer/authorizer.hpp (lines 83 - 84)


"The `principal` and `role` parameters are packed in the request." seems 
superfluous to the summary, and is already included in the 'request' @param. 
Delete this sentence.



include/mesos/authorizer/authorizer.hpp (line 100)


Kill the "packed in" sentence for all of these. That info is in the request 
@param comment.



include/mesos/authorizer/authorizer.hpp (lines 102 - 104)


How about just "`ACL::RunTask` packing all the parameters..."? We can 
figure out that it's an instance of a protobuf message.



include/mesos/authorizer/authorizer.hpp (lines 107 - 109)


You can probably shorten this here and everywhere by just saying "A failed 
future indicates a problem processing the request; the request can be retried."



include/mesos/authorizer/authorizer.hpp (line 119)


s/RunTask/ShutdownFramework/



include/mesos/authorizer/authorizer.hpp (line 124)


s/ran/registered/



include/mesos/authorizer/authorizer.hpp (line 133)


s/reserve particular resources/reserve resources/ since the only values 
currently allowed for `resources` are ANY or NONE.



include/mesos/authorizer/authorizer.hpp (line 138)


s/reserve one or more types of resources/reserve resources/



include/mesos/authorizer/authorizer.hpp (lines 140 - 141)


s/reserve the types of resources contained in the request/reserve resources/


- Adam B


On Dec. 16, 2015, 5:03 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41444/
> ---
> 
> (Updated Dec. 16, 2015, 5:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Greg Mann, Jan Schlicht, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extract a repetitive part of the function comments into a class comment. 
> Added backticks, quotes when necessary, formatted comments to avoid 
> jaggedness.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
>   src/master/main.cpp e00f878770f3e0bddae5a137b50a00822d154e2c 
> 
> Diff: https://reviews.apache.org/r/41444/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>