Re: Review Request 40544: Added quota remove handling.

2015-12-21 Thread Joerg Schad 

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

(Updated Dec. 21, 2015, 6:54 p.m.)


Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


Changes
---

Removed Dependency


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


Repository: mesos


Description
---

Added quota remove handling.


Diffs
-

  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

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


Testing
---

Test are in the next review.


Thanks,

Joerg Schad

Re: Review Request 40544: Added quota remove handling.

2015-11-25 Thread Anand Mazumdar 

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


1. General fly by style comments.
2. Have we thought about moving the request validations to a central place 
preferably in `src/master/validation.cpp` ?


src/master/master.hpp (line 861)


We generally avoid redundant/self-explanatory comments. Can we remove this ?



src/master/quota_handler.cpp (line 362)


Can we put the `request.url.path` in Quotes ('') similar to other instances 
below ?



src/master/quota_handler.cpp (line 374)


When can a valid request path have more then 3 tokens ?

I wonder if it would just be fine for this to be exact equality check for 3 
parts i.e. `!=3` ?



src/master/quota_handler.cpp (line 377)


Let's not return speculative errors back to the end-user. How about:

": Expected atleast 3 but found only " + tokens.size() + " tokens"



src/master/quota_handler.cpp (line 383)


Remove period at the end.

Also how about: ": Missing 'quota' endpoint"  ?



src/master/quota_handler.cpp (line 386)


`const string&`

Also, what happens if this is an unknown role for the master ?



src/master/quota_handler.cpp (line 391)


1. Remove period at the end.
2. s/Can not remove quota for a role that has no quota/Role has no quota set



src/master/quota_handler.cpp (line 394)


Kill this redundant comment.


- Anand Mazumdar


On Nov. 22, 2015, 1:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40544/
> ---
> 
> (Updated Nov. 22, 2015, 1:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added quota remove handling.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40544/diff/
> 
> 
> Testing
> ---
> 
> Test are in the next review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40544: Added quota remove handling.

2015-11-25 Thread Alexander Rukletsov 

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


Please adjust the JIRA ticket: MESOS-4014.

- Alexander Rukletsov


On Nov. 22, 2015, 1:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40544/
> ---
> 
> (Updated Nov. 22, 2015, 1:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added quota remove handling.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40544/diff/
> 
> 
> Testing
> ---
> 
> Test are in the next review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40544: Added quota remove handling.

2015-11-25 Thread Klaus Ma 

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



src/master/quota_handler.cpp (lines 372 - 386)


I think whether we need a general url tools for suck similar action for 
API? for example, for the `tokenize`, what's the behavior if there are two '/'?


- Klaus Ma


On Nov. 26, 2015, 5:26 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40544/
> ---
> 
> (Updated Nov. 26, 2015, 5:26 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4014
> https://issues.apache.org/jira/browse/MESOS-4014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added quota remove handling.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40544/diff/
> 
> 
> Testing
> ---
> 
> Test are in the next review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40544: Added quota remove handling.

2015-11-25 Thread Joris Van Remoortere 

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

Ship it!


Minor nits, and addressed comments before committing.

- Joris Van Remoortere


On Nov. 25, 2015, 9:26 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40544/
> ---
> 
> (Updated Nov. 25, 2015, 9:26 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4014
> https://issues.apache.org/jira/browse/MESOS-4014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added quota remove handling.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40544/diff/
> 
> 
> Testing
> ---
> 
> Test are in the next review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40544: Added quota remove handling.

2015-11-25 Thread Joerg Schad 

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

(Updated Nov. 25, 2015, 9:26 p.m.)


Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


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


Repository: mesos


Description
---

Added quota remove handling.


Diffs
-

  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

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


Testing
---

Test are in the next review.


Thanks,

Joerg Schad

Re: Review Request 40544: Added quota remove handling.

2015-11-25 Thread Joris Van Remoortere 


> On Nov. 23, 2015, 8:43 a.m., Qian Zhang wrote:
> > src/master/quota_handler.cpp, line 387
> > 
> >
> > I think we need to check if the role is known by master just like what 
> > we did for set quota, and also need to add a TODO to state that we should 
> > remove this check once we are able to dynamically add roles.

I agree with the check in the master, as that is slightly different from the 
implied check currently being relied on (that a quota is set for that role).
I disagree on the TODO. I don't think it makes sense to remove this check once 
dynamic roles are introduced, as removing quota for a role doesn't seem to make 
sense for a role that doesn't exist, even in a dynamic role world.


- Joris


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


On Nov. 25, 2015, 9:26 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40544/
> ---
> 
> (Updated Nov. 25, 2015, 9:26 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4014
> https://issues.apache.org/jira/browse/MESOS-4014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added quota remove handling.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40544/diff/
> 
> 
> Testing
> ---
> 
> Test are in the next review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40544: Added quota remove handling.

2015-11-23 Thread Qian Zhang 

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



src/master/quota_handler.cpp (line 387)


I think we need to check if the role is known by master just like what we 
did for set quota, and also need to add a TODO to state that we should remove 
this check once we are able to dynamically add roles.


- Qian Zhang


On Nov. 22, 2015, 9:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40544/
> ---
> 
> (Updated Nov. 22, 2015, 9:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added quota remove handling.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40544/diff/
> 
> 
> Testing
> ---
> 
> Test are in the next review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40544: Added quota remove handling.

2015-11-22 Thread Joerg Schad 

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

(Updated Nov. 22, 2015, 1:35 p.m.)


Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


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


Repository: mesos


Description
---

Added quota remove handling.


Diffs (updated)
-

  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

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


Testing
---

Test are in the next review.


Thanks,

Joerg Schad