You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2015/12/17 15:52:56 UTC
Review Request 41514: Accepted a single JSON object for quota set
request.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
Bugs: MESOS-3960
https://issues.apache.org/jira/browse/MESOS-3960
Repository: mesos
Description
-------
POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
Diffs
-----
src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
Diff: https://reviews.apache.org/r/41514/diff/
Testing
-------
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Dec. 21, 2015, 10:03 p.m., Guangya Liu wrote:
> > src/master/quota_handler.cpp, lines 358-359
> > <https://reviews.apache.org/r/41514/diff/2/?file=1173817#file1173817line358>
> >
> > What will happen if end user input TRUE, True, FALSE, False? Shall we document those keyword in quota documentation?
We lower-case the input before checking with this allowed set.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111543
-----------------------------------------------------------
On Dec. 21, 2015, 1:27 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2015, 1:27 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111543
-----------------------------------------------------------
src/master/quota_handler.cpp (lines 333 - 334)
<https://reviews.apache.org/r/41514/#comment171739>
What will happen if end user input TRUE, True, FALSE, False? Shall we document those keyword in quota documentation?
- Guangya Liu
On 十二月 21, 2015, 1:27 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated 十二月 21, 2015, 1:27 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Dec. 22, 2015, 7:48 p.m., Joris Van Remoortere wrote:
> > src/master/quota_handler.cpp, lines 357-359
> > <https://reviews.apache.org/r/41514/diff/2/?file=1173817#file1173817line357>
> >
> > Have you considered making this static?
> > What about building it up from the true and false components so that you can use the true component below. Something along these lines:
> > ```
> > const static hashset<string> trueValues({"true", "yes", "t", "y"});
> > const static hashset<string> falseValues({"false", "no", "f", "n"});
> > const static hashset<string> allowedForceValues = trueValues + falseValues;
> > ```
> > and
> > ```
> > const bool forced = (force.isSome() && trueValues.contains(forceValue));
> > ```
> >
> > Please note the indentation is off the L334 in the current diff.
Review has already been updated to use JSON::Boolean.
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111638
-----------------------------------------------------------
On Dec. 22, 2015, 6:09 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to
> key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111638
-----------------------------------------------------------
Ship it!
src/master/quota_handler.cpp (lines 332 - 334)
<https://reviews.apache.org/r/41514/#comment171850>
Have you considered making this static?
What about building it up from the true and false components so that you can use the true component below. Something along these lines:
```
const static hashset<string> trueValues({"true", "yes", "t", "y"});
const static hashset<string> falseValues({"false", "no", "f", "n"});
const static hashset<string> allowedForceValues = trueValues + falseValues;
```
and
```
const bool forced = (force.isSome() && trueValues.contains(forceValue));
```
Please note the indentation is off the L334 in the current diff.
- Joris Van Remoortere
On Dec. 22, 2015, 6:09 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to
> key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/
-----------------------------------------------------------
(Updated Dec. 22, 2015, 6:09 p.m.)
Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
Changes
-------
Removed an unnecessary include; moved adding a necessaru include to a follow-up cleanup patch.
Bugs: MESOS-3960
https://issues.apache.org/jira/browse/MESOS-3960
Repository: mesos
Description (updated)
-------
POST request to "/quota" requires a single JSON object as opposed to
key-value pairs encoded in a string.
Diffs (updated)
-----
src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
Diff: https://reviews.apache.org/r/41514/diff/
Testing
-------
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 22, 2015, 5:47 p.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 30
> > <https://reviews.apache.org/r/41514/diff/3/?file=1174490#file1174490line30>
> >
> > Remove this. This must have crept in due to handling multiple `force` field values ?
You're right and that's embarrassing. Thanks a lot for catching this!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111624
-----------------------------------------------------------
On Dec. 22, 2015, 11:02 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2015, 11:02 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111624
-----------------------------------------------------------
Ship it!
LGTM !
src/master/quota_handler.cpp (line 30)
<https://reviews.apache.org/r/41514/#comment171831>
Remove this. This must have crept in due to handling multiple `force` field values ?
- Anand Mazumdar
On Dec. 22, 2015, 11:02 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 22, 2015, 11:02 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/
-----------------------------------------------------------
(Updated Dec. 22, 2015, 11:02 a.m.)
Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
Changes
-------
Used `JSON::Boolean` for the `force` field.
Bugs: MESOS-3960
https://issues.apache.org/jira/browse/MESOS-3960
Repository: mesos
Description
-------
POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
Diffs (updated)
-----
src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
Diff: https://reviews.apache.org/r/41514/diff/
Testing
-------
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/
-----------------------------------------------------------
(Updated Dec. 21, 2015, 1:27 p.m.)
Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
Changes
-------
Addressed comments and rebased.
Bugs: MESOS-3960
https://issues.apache.org/jira/browse/MESOS-3960
Repository: mesos
Description
-------
POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
Diffs (updated)
-----
src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
Diff: https://reviews.apache.org/r/41514/diff/
Testing
-------
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 353
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line353>
> >
> > hmmm .. Should we return a `BadRequest` for all other non-allowed values of `force` other then `true` or `false` ?
>
> Alexander Rukletsov wrote:
> I was thinking about the [Postel's law](https://en.wikipedia.org/wiki/Robustness_principle), but maybe you are right and we should not interpret "force:t", "force:1", "force:god-damn-yes" as `false`, I'll fix that.
>
> Anand Mazumdar wrote:
> I was wondering if we should accept only `true/false` as valid values. Currently the `/scheduler` endpoint also specifies a semantically similar field `force` in `Call::Subscribe` message that just takes these 2 possible values. Should we strive for consistency across all the endpoints and reject all other values ?
I agree consistency is important, thanks for spotting this. I'll update the review shortly.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111086
-----------------------------------------------------------
On Dec. 21, 2015, 1:27 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2015, 1:27 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 299
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line299>
> >
> > hmm, why don't we do a `using google::protobuf::RepeatedPtrField` and get rid of all this jaggedness ?
>
> Alexander Rukletsov wrote:
> Yeah, and also `#include` it properly : )
Fixed in https://reviews.apache.org/r/41603/
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/tests/master_quota_tests.cpp, line 199
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168676#file1168676line199>
> >
> > Not related to this review : Why don't we make all these `badRequest` expected strings `const` ?
>
> Alexander Rukletsov wrote:
> Yeah, let's fix it. Thanks!
Cleaned up in https://reviews.apache.org/r/41603/
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111086
-----------------------------------------------------------
On Dec. 21, 2015, 1:27 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2015, 1:27 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > LGTM.. Just some nits around:
> >
> > - using `const` for test strings.
> > - reducing jaggedness for some blocks.
> >
> > Also, a query regarding just accepting `true/false` as `force` field values.
Good points, thanks for the review, Anand!
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 273
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line273>
> >
> > nit: use backticks (`force`)
> > s/flag/field
I'm not sure we backtick flags, do we? AFAIK, we do that for object, variables, function names, types.
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 299
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line299>
> >
> > hmm, why don't we do a `using google::protobuf::RepeatedPtrField` and get rid of all this jaggedness ?
Yeah, and also `#include` it properly : )
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 338
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line338>
> >
> > I like the less jagged version more here. It's also more consistent with other similar strings in the function. What do you think ?
It's a known fact, that jaggedness is very subjective. I would argue, that wrapping error message after `BadRequest` is less jagged and more readable, because more there is more space for the message itself. Consistency is important, hence the following review: https://reviews.apache.org/r/41515/
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 353
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line353>
> >
> > hmmm .. Should we return a `BadRequest` for all other non-allowed values of `force` other then `true` or `false` ?
I was thinking about the [Postel's law](https://en.wikipedia.org/wiki/Robustness_principle), but maybe you are right and we should not interpret "force:t", "force:1", "force:god-damn-yes" as `false`, I'll fix that.
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/tests/master_quota_tests.cpp, line 199
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168676#file1168676line199>
> >
> > Not related to this review : Why don't we make all these `badRequest` expected strings `const` ?
Yeah, let's fix it. Thanks!
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111086
-----------------------------------------------------------
On Dec. 17, 2015, 2:52 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2015, 2:52 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 353
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line353>
> >
> > hmmm .. Should we return a `BadRequest` for all other non-allowed values of `force` other then `true` or `false` ?
>
> Alexander Rukletsov wrote:
> I was thinking about the [Postel's law](https://en.wikipedia.org/wiki/Robustness_principle), but maybe you are right and we should not interpret "force:t", "force:1", "force:god-damn-yes" as `false`, I'll fix that.
>
> Anand Mazumdar wrote:
> I was wondering if we should accept only `true/false` as valid values. Currently the `/scheduler` endpoint also specifies a semantically similar field `force` in `Call::Subscribe` message that just takes these 2 possible values. Should we strive for consistency across all the endpoints and reject all other values ?
>
> Alexander Rukletsov wrote:
> I agree consistency is important, thanks for spotting this. I'll update the review shortly.
The best solution here is to use `JSON::Boolean` I think.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111086
-----------------------------------------------------------
On Dec. 21, 2015, 1:27 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2015, 1:27 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 353
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line353>
> >
> > hmmm .. Should we return a `BadRequest` for all other non-allowed values of `force` other then `true` or `false` ?
>
> Alexander Rukletsov wrote:
> I was thinking about the [Postel's law](https://en.wikipedia.org/wiki/Robustness_principle), but maybe you are right and we should not interpret "force:t", "force:1", "force:god-damn-yes" as `false`, I'll fix that.
I was wondering if we should accept only `true/false` as valid values. Currently the `/scheduler` endpoint also specifies a semantically similar field `force` in `Call::Subscribe` message that just takes these 2 possible values. Should we strive for consistency across all the endpoints and reject all other values ?
> On Dec. 18, 2015, 12:04 a.m., Anand Mazumdar wrote:
> > src/master/quota_handler.cpp, line 338
> > <https://reviews.apache.org/r/41514/diff/1/?file=1168675#file1168675line338>
> >
> > I like the less jagged version more here. It's also more consistent with other similar strings in the function. What do you think ?
>
> Alexander Rukletsov wrote:
> It's a known fact, that jaggedness is very subjective. I would argue, that wrapping error message after `BadRequest` is less jagged and more readable, because more there is more space for the message itself. Consistency is important, hence the following review: https://reviews.apache.org/r/41515/
sgtm, I dropped the issue.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111086
-----------------------------------------------------------
On Dec. 21, 2015, 1:27 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2015, 1:27 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 0217149a865ede751b3a03fe40b2d91b487b7b10
> src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1
> src/tests/role_tests.cpp 2c5f68ccaac7e9a37345e2f331d1bc35cae77736
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 41514: Accepted a single JSON object for quota set
request.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41514/#review111086
-----------------------------------------------------------
LGTM.. Just some nits around:
- using `const` for test strings.
- reducing jaggedness for some blocks.
Also, a query regarding just accepting `true/false` as `force` field values.
src/master/quota_handler.cpp (line 250)
<https://reviews.apache.org/r/41514/#comment171067>
nit: use backticks (`force`)
s/flag/field
src/master/quota_handler.cpp (line 274)
<https://reviews.apache.org/r/41514/#comment171066>
hmm, why don't we do a `using google::protobuf::RepeatedPtrField` and get rid of all this jaggedness ?
src/master/quota_handler.cpp (line 313)
<https://reviews.apache.org/r/41514/#comment171064>
I like the less jagged version more here. It's also more consistent with other similar strings in the function. What do you think ?
src/master/quota_handler.cpp (line 328)
<https://reviews.apache.org/r/41514/#comment171062>
hmmm .. Should we return a `BadRequest` for all other non-allowed values of `force` other then `true` or `false` ?
src/tests/master_quota_tests.cpp (line 116)
<https://reviews.apache.org/r/41514/#comment171059>
nit: `const string`
src/tests/master_quota_tests.cpp (line 122)
<https://reviews.apache.org/r/41514/#comment171060>
nit: `const string`
src/tests/master_quota_tests.cpp (line 199)
<https://reviews.apache.org/r/41514/#comment171061>
Not related to this review : Why don't we make all these `badRequest` expected strings `const` ?
- Anand Mazumdar
On Dec. 17, 2015, 2:52 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2015, 2:52 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
>
>
> Repository: mesos
>
>
> Description
> -------
>
> POST request to "/quota" requires a single JSON object as opposed to key-value pairs encoded in a string.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272
> src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74
>
> Diff: https://reviews.apache.org/r/41514/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>