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 2016/01/18 23:49:52 UTC
Review Request 42476: Introduced protobuf for set quota requests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
Bugs: MESOS-4410
https://issues.apache.org/jira/browse/MESOS-4410
Repository: mesos
Description
-------
See summary.
Diffs
-----
docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6
include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d
src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d
src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
Diff: https://reviews.apache.org/r/42476/diff/
Testing
-------
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/#review115092
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42477, 42476]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 18, 2016, 11:37 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 11:37 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6
> include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
> src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d
> src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d
> src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
>
> Diff: https://reviews.apache.org/r/42476/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 280
> > <https://reviews.apache.org/r/42476/diff/2/?file=1200760#file1200760line280>
> >
> > Not an issue, but question/remark: the protoRequest.error() is most likely to be less concise compared to the previous error messages?
Do you think we should hide the real error from an operator? I'd rather have the precise error which an operator can show me on irc or user list : ).
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 54
> > <https://reviews.apache.org/r/42476/diff/2/?file=1200759#file1200759line54>
> >
> > Given the naming scheme QuotaStatus wouldn't a more consistent name be QuotaSet?
The question here is whether we will be reusing the same protobuf for, say, update requests? My feeling is that this should be possible, hence a more general naming.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/#review115127
-----------------------------------------------------------
On Jan. 18, 2016, 11:37 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 11:37 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6
> include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
> src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d
> src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d
> src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
>
> Diff: https://reviews.apache.org/r/42476/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 56
> > <https://reviews.apache.org/r/42476/diff/2/?file=1200759#file1200759line56>
> >
> > I feel that optimizing here (with = 16) is more confusing than helpful, especially as this protobuf is mostly used as the schema for the json request.
>
> Joris Van Remoortere wrote:
> What are we optimizing here? Room for more fields?
>
> Alexander Rukletsov wrote:
> Yep, future fields.
>
> "Tag numbers 1-15 require one less byte to encode than higher numbers, so as an optimization you can decide to use those tags for the commonly used or repeated elements, leaving tags 16 and higher for less-commonly used optional elements. Each element in a repeated field requires re-encoding the tag number, so repeated fields are particularly good candidates for this optimization."
>
> https://developers.google.com/protocol-buffers/docs/cpptutorial?hl=en
After an offline conversation with Joris we decided not to introduce this optimization.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/#review115127
-----------------------------------------------------------
On Jan. 26, 2016, 4:33 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 4:33 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/quota.md 208bfa06a9fc50843439bab9a041ccb557657b5d
> include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
> src/master/quota.cpp 698150487da71e9dcd907d471067d62ff8a201b4
> src/master/quota_handler.cpp aa06cbc83d2c467c2422f1d27bb9507998953c44
> src/tests/master_quota_tests.cpp 7d07be355b9323f2a7f27f84020a6ecc7f061140
> src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
>
> Diff: https://reviews.apache.org/r/42476/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 56
> > <https://reviews.apache.org/r/42476/diff/2/?file=1200759#file1200759line56>
> >
> > I feel that optimizing here (with = 16) is more confusing than helpful, especially as this protobuf is mostly used as the schema for the json request.
What are we optimizing here? Room for more fields?
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/#review115127
-----------------------------------------------------------
On Jan. 22, 2016, 3:43 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 3:43 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/quota.md 208bfa06a9fc50843439bab9a041ccb557657b5d
> include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
> src/master/quota_handler.cpp aa06cbc83d2c467c2422f1d27bb9507998953c44
> src/tests/master_quota_tests.cpp 7d07be355b9323f2a7f27f84020a6ecc7f061140
> src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
>
> Diff: https://reviews.apache.org/r/42476/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 56
> > <https://reviews.apache.org/r/42476/diff/2/?file=1200759#file1200759line56>
> >
> > I feel that optimizing here (with = 16) is more confusing than helpful, especially as this protobuf is mostly used as the schema for the json request.
>
> Joris Van Remoortere wrote:
> What are we optimizing here? Room for more fields?
Yep, future fields.
"Tag numbers 1-15 require one less byte to encode than higher numbers, so as an optimization you can decide to use those tags for the commonly used or repeated elements, leaving tags 16 and higher for less-commonly used optional elements. Each element in a repeated field requires re-encoding the tag number, so repeated fields are particularly good candidates for this optimization."
https://developers.google.com/protocol-buffers/docs/cpptutorial?hl=en
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/#review115127
-----------------------------------------------------------
On Jan. 22, 2016, 3:43 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 3:43 p.m.)
>
>
> Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/quota.md 208bfa06a9fc50843439bab9a041ccb557657b5d
> include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
> src/master/quota_handler.cpp aa06cbc83d2c467c2422f1d27bb9507998953c44
> src/tests/master_quota_tests.cpp 7d07be355b9323f2a7f27f84020a6ecc7f061140
> src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
>
> Diff: https://reviews.apache.org/r/42476/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Joerg Schad <jo...@mesosphere.io>.
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 54
> > <https://reviews.apache.org/r/42476/diff/2/?file=1200759#file1200759line54>
> >
> > Given the naming scheme QuotaStatus wouldn't a more consistent name be QuotaSet?
>
> Alexander Rukletsov wrote:
> The question here is whether we will be reusing the same protobuf for, say, update requests? My feeling is that this should be possible, hence a more general naming.
It is ok with me, but it would be different for the Status and Remove Request (which are also subtypes of Requests).
> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 280
> > <https://reviews.apache.org/r/42476/diff/2/?file=1200760#file1200760line280>
> >
> > Not an issue, but question/remark: the protoRequest.error() is most likely to be less concise compared to the previous error messages?
>
> Alexander Rukletsov wrote:
> Do you think we should hide the real error from an operator? I'd rather have the precise error which an operator can show me on irc or user list : ).
I said not an issue :-), I just wanted to note that before we added more semantic meaning to the error string (as we checked for the error ourself) and the protobuf error will be most likely less precise.
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/#review115127
-----------------------------------------------------------
On Jan. 18, 2016, 11:37 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 11:37 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6
> include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
> src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d
> src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d
> src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
>
> Diff: https://reviews.apache.org/r/42476/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/#review115127
-----------------------------------------------------------
Ship it!
include/mesos/quota/quota.proto (line 53)
<https://reviews.apache.org/r/42476/#comment175991>
Does it make sense to use this protobuf as well in master_quota_tests.cpp::createRequestBody?
include/mesos/quota/quota.proto (line 54)
<https://reviews.apache.org/r/42476/#comment175985>
Given the naming scheme QuotaStatus wouldn't a more consistent name be QuotaSet?
include/mesos/quota/quota.proto (line 56)
<https://reviews.apache.org/r/42476/#comment175989>
I feel that optimizing here (with = 16) is more confusing than helpful, especially as this protobuf is mostly used as the schema for the json request.
src/master/quota_handler.cpp (line 246)
<https://reviews.apache.org/r/42476/#comment175990>
Not an issue, but question/remark: the protoRequest.error() is most likely to be less concise compared to the previous error messages?
- Joerg Schad
On Jan. 18, 2016, 11:37 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> -----------------------------------------------------------
>
> (Updated Jan. 18, 2016, 11:37 p.m.)
>
>
> Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
>
>
> Bugs: MESOS-4410
> https://issues.apache.org/jira/browse/MESOS-4410
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6
> include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
> src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d
> src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d
> src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
>
> Diff: https://reviews.apache.org/r/42476/diff/
>
>
> Testing
> -------
>
> make check on Mac OS 10.10.4
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/
-----------------------------------------------------------
(Updated Jan. 19, 2016, 3:06 p.m.)
Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
Changes
-------
Jörg's comments.
Bugs: MESOS-4410
https://issues.apache.org/jira/browse/MESOS-4410
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6
include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d
src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d
src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
Diff: https://reviews.apache.org/r/42476/diff/
Testing
-------
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov
Re: Review Request 42476: Introduced protobuf for set quota requests.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42476/
-----------------------------------------------------------
(Updated Jan. 18, 2016, 11:37 p.m.)
Review request for mesos, Ben Mahler, Joerg Schad, and Joris Van Remoortere.
Changes
-------
Rebased.
Bugs: MESOS-4410
https://issues.apache.org/jira/browse/MESOS-4410
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
docs/quota.md 1a6d2f07fb74d168a7eb30764ab9ff80cea5e3b6
include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79
src/master/quota_handler.cpp f44736cd5849d4fb22a75c1238d433a1c0c9708d
src/tests/master_quota_tests.cpp e8cb074c2913cafdc6b1792896f29e53f1210c9d
src/tests/role_tests.cpp 979391306e2427aaa63a5df32704913f79e20e36
Diff: https://reviews.apache.org/r/42476/diff/
Testing
-------
make check on Mac OS 10.10.4
Thanks,
Alexander Rukletsov