Re: Review Request 42476: Introduced protobuf for set quota requests.

2016-01-26 Thread Alexander Rukletsov 


> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 56
> > 
> >
> > 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.

2016-01-26 Thread Alexander Rukletsov 


> On Jan. 25, 2016, 8:40 a.m., Joris Van Remoortere wrote:
> > include/mesos/quota/quota.proto, line 56
> > 
> >
> > I don't get the `16` as per Joerg's comment.

Commented on above.


> On Jan. 25, 2016, 8:40 a.m., Joris Van Remoortere wrote:
> > src/master/quota_handler.cpp, line 68
> > 
> >
> > Can you augment the `QuotaInfo` validation function with the new role 
> > validation introduced?

Sure.


> On Jan. 25, 2016, 8:40 a.m., Joris Van Remoortere wrote:
> > src/master/quota_handler.cpp, lines 67-69
> > 
> >
> > Please keep this function a general use one. Users of it should not 
> > need to create a `QuotaRequest` in order to construct a `QuotaInfo`.

I think we agreed to have `*Request` and `*Info` protobufs for request and 
storage respectively, and a converion function. If someone wants to construct 
`QuotaInfo`, why not doing it manually?


- Alexander


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


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.

2016-01-26 Thread Alexander Rukletsov 

---
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.


Changes
---

Added role validation.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  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.

2016-01-26 Thread Mesos ReviewBot 

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



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. 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.

2016-01-26 Thread Alexander Rukletsov 


> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 56
> > 
> >
> > 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.

2016-01-26 Thread Alexander Rukletsov 

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

(Updated Jan. 26, 2016, 8:48 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
Van Remoortere.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/quota.md 208bfa06a9fc50843439bab9a041ccb557657b5d 
  include/mesos/quota/quota.proto 338412ee967e14aa1957a47f4a50f2e19e4eca79 
  src/master/quota.hpp 9ac390777946047978a00aa59082e78ce3997441 
  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.

2016-01-26 Thread Joris Van Remoortere 

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


Fix it, then Ship it!





src/master/quota.cpp (lines 94 - 95)


Wrapping.


- Joris Van Remoortere


On Jan. 26, 2016, 8:48 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, 8:48 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.hpp 9ac390777946047978a00aa59082e78ce3997441 
>   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.

2016-01-25 Thread Joris Van Remoortere 


> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 56
> > 
> >
> > 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.

2016-01-25 Thread Joris Van Remoortere 


> On Jan. 23, 2016, 5:27 a.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 77
> > 
> >
> > Do we need to check `has_role()`?

It will just be an empty role if it was not set.


- Joris


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


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.

2016-01-25 Thread Joris Van Remoortere 

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


Fix it, then Ship it!





include/mesos/quota/quota.proto (line 56)


I don't get the `16` as per Joerg's comment.



src/master/quota_handler.cpp (lines 67 - 68)


Please keep this function a general use one. Users of it should not need to 
create a `QuotaRequest` in order to construct a `QuotaInfo`.



src/master/quota_handler.cpp (line 68)


Can you augment the `QuotaInfo` validation function with the new role 
validation introduced?


- Joris Van Remoortere


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.

2016-01-22 Thread Mesos ReviewBot 

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



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. 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.

2016-01-21 Thread Bernd Mathiske 

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

Ship it!


After addressing Joerg's remaining issues, ship it!

- Bernd Mathiske


On Jan. 20, 2016, 4:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 4:12 a.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 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.

2016-01-20 Thread Bernd Mathiske 

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



docs/quota.md (line 94)


plural



include/mesos/quota/quota.proto (line 58)


"should" does not carry the right meaning here.

// The role for which to set quota guarantees.



include/mesos/quota/quota.proto (line 61)


// Resources to be guaranteed as allocatable for the above role.



include/mesos/quota/quota.proto (line 62)


plural



src/master/quota_handler.cpp (line 283)


Why not use protoRequest here?


- Bernd Mathiske


On Jan. 20, 2016, 1 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 1 a.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 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.

2016-01-20 Thread Alexander Rukletsov 


> On Jan. 20, 2016, 9:14 a.m., Bernd Mathiske wrote:
> > docs/quota.md, line 94
> > 
> >
> > plural

I think we use single everywhere. It means total quota guarantee and shouldn't 
necessarily resemble the fact that there are multiple resources objects backing 
it. Moreover, it will require us to update `QuotaInfo` and `QuotaRequest` 
protos as well.


> On Jan. 20, 2016, 9:14 a.m., Bernd Mathiske wrote:
> > include/mesos/quota/quota.proto, line 62
> > 
> >
> > plural

See above.


> On Jan. 20, 2016, 9:14 a.m., Bernd Mathiske wrote:
> > src/master/quota_handler.cpp, line 327
> > 
> >
> > Why not use protoRequest here?

Good catch, thanks!


- Alexander


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


On Jan. 20, 2016, 9 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 9 a.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 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.

2016-01-20 Thread Bernd Mathiske 


> On Jan. 20, 2016, 1:14 a.m., Bernd Mathiske wrote:
> > docs/quota.md, line 94
> > 
> >
> > plural
> 
> Alexander Rukletsov wrote:
> I think we use single everywhere. It means total quota guarantee and 
> shouldn't necessarily resemble the fact that there are multiple resources 
> objects backing it. Moreover, it will require us to update `QuotaInfo` and 
> `QuotaRequest` protos as well.

I would not have seen it this way if the comment describing the field in 
QuotaRequest did not make me believe this was plural. Now I see your point. 
However, in that case, I would suggest making this perspectiove clear at the 
field definition's comment. Maybe:

// The requested guarantee that these resources will be allocatable for the 
above role.


- Bernd


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


On Jan. 20, 2016, 3:58 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 3:58 a.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 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.

2016-01-20 Thread Alexander Rukletsov 


> On Jan. 20, 2016, 9:14 a.m., Bernd Mathiske wrote:
> > docs/quota.md, line 94
> > 
> >
> > plural
> 
> Alexander Rukletsov wrote:
> I think we use single everywhere. It means total quota guarantee and 
> shouldn't necessarily resemble the fact that there are multiple resources 
> objects backing it. Moreover, it will require us to update `QuotaInfo` and 
> `QuotaRequest` protos as well.
> 
> Bernd Mathiske wrote:
> I would not have seen it this way if the comment describing the field in 
> QuotaRequest did not make me believe this was plural. Now I see your point. 
> However, in that case, I would suggest making this perspectiove clear at the 
> field definition's comment. Maybe:
> 
> // The requested guarantee that these resources will be allocatable 
> for the above role.

Good idea, I'll update the comment.

My biggest concern is that "guarantees" sounds misleading to me. There is a 
single guarantee per role, which may comprise various resources.

Moreover, though we indeed almost always prefer plural for repeated fields, 
singular is considered a good practice as well: 
https://developers.google.com/protocol-buffers/docs/cpptutorial


- Alexander


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


On Jan. 20, 2016, 11:58 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42476/
> ---
> 
> (Updated Jan. 20, 2016, 11:58 a.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 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.

2016-01-20 Thread Alexander Rukletsov 

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

(Updated Jan. 20, 2016, 12:12 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, Joerg Schad, and Joris 
Van Remoortere.


Changes
---

Updated 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.

2016-01-19 Thread Joerg Schad 

---
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)


Does it make sense to use this protobuf as well in 
master_quota_tests.cpp::createRequestBody?



include/mesos/quota/quota.proto (line 54)


Given the naming scheme QuotaStatus wouldn't a more consistent name be 
QuotaSet?



include/mesos/quota/quota.proto (line 56)


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)


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.

2016-01-19 Thread Alexander Rukletsov 


> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 280
> > 
> >
> > 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
> > 
> >
> > 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.

2016-01-19 Thread Joerg Schad 


> On Jan. 19, 2016, 8:37 a.m., Joerg Schad wrote:
> > include/mesos/quota/quota.proto, line 54
> > 
> >
> > 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
> > 
> >
> > 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.

2016-01-19 Thread Alexander Rukletsov 

---
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.

2016-01-18 Thread Alexander Rukletsov 

---
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

Re: Review Request 42476: Introduced protobuf for set quota requests.

2016-01-18 Thread Mesos ReviewBot 

---
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
> 
>

Review Request 42476: Introduced protobuf for set quota requests.

2016-01-18 Thread Alexander Rukletsov 

---
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