You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joerg Schad <jo...@mesosphere.io> on 2015/11/22 14:35:12 UTC
Re: Review Request 40544: Added quota remove handling.
-----------------------------------------------------------
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
Re: Review Request 40544: Added quota remove handling.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/40544/#comment167351>
We generally avoid redundant/self-explanatory comments. Can we remove this ?
src/master/quota_handler.cpp (line 362)
<https://reviews.apache.org/r/40544/#comment167354>
Can we put the `request.url.path` in Quotes ('') similar to other instances below ?
src/master/quota_handler.cpp (line 374)
<https://reviews.apache.org/r/40544/#comment167364>
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)
<https://reviews.apache.org/r/40544/#comment167353>
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)
<https://reviews.apache.org/r/40544/#comment167357>
Remove period at the end.
Also how about: ": Missing 'quota' endpoint" ?
src/master/quota_handler.cpp (line 386)
<https://reviews.apache.org/r/40544/#comment167352>
`const string&`
Also, what happens if this is an unknown role for the master ?
src/master/quota_handler.cpp (line 391)
<https://reviews.apache.org/r/40544/#comment167358>
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)
<https://reviews.apache.org/r/40544/#comment167359>
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.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/40544/#comment167464>
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.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
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.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Nov. 23, 2015, 8:43 a.m., Qian Zhang wrote:
> > src/master/quota_handler.cpp, line 387
> > <https://reviews.apache.org/r/40544/diff/2/?file=1135399#file1135399line387>
> >
> > 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.
Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40544/#review107565
-----------------------------------------------------------
src/master/quota_handler.cpp (line 387)
<https://reviews.apache.org/r/40544/#comment166788>
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
>
>