Re: Review Request 39285: Added Quota Request Validation.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Nov. 12, 2015, 3:21 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 12, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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



src/master/quota_handler.cpp (lines 95 - 102)


Not your fault, but here is one thing that bothers me a bit.

It looks like we are inconsistent about what we expect for operator 
endpoints. For example, dynamic reservations take a string 
"slaveId==", while maintenance expect "" directly.

I would like us to be more consistent on this. Shall we create a clean-up 
ticket?


- Alexander Rukletsov


On Nov. 12, 2015, 3:21 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 12, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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



src/master/quota_handler.cpp (lines 107 - 111)


We may be able to extract these—together with some other parsing—lines into 
a separate method.


- Alexander Rukletsov


On Nov. 12, 2015, 3:21 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 12, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

> On Nov. 13, 2015, 12:03 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 95-102
> > 
> >
> > Not your fault, but here is one thing that bothers me a bit.
> > 
> > It looks like we are inconsistent about what we expect for operator 
> > endpoints. For example, dynamic reservations take a string 
> > "slaveId==", while maintenance expect "" 
> > directly.
> > 
> > I would like us to be more consistent on this. Shall we create a 
> > clean-up ticket?

https://issues.apache.org/jira/browse/MESOS-3914


- Alexander


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


On Nov. 12, 2015, 3:21 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 12, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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



src/master/master.hpp (line 878)


Can be static.


- Joerg Schad


On Nov. 12, 2015, 3:21 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 12, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 12, 2015, 10:26 a.m.)


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


Changes
---

Adressed (offline) comments.


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joris Van Remoortere]

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



src/master/quota_handler.cpp (line 114)


Missing a negation



src/master/quota_handler.cpp (line 125)


trim the extra `to`



src/master/quota_handler.cpp (line 126)


we can still rename `extract` to `create`



src/master/quota_handler.cpp (line 155)


Can not set quota for a role that already has quota



src/master/quota.cpp (line 45)


how about `.empty()`?



src/master/quota_handler.cpp (line 53)


Let's rename this to `request`



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


After discussing this more, I'd really love to clean this up and leverage 
the validation routine above (which we do in set).
Let's do something like:
```
QuotaInfo quota;
quota.mutable_guarantee()->CopyFrom(resources.get());
if (!resources.get().empty()) {
  quota.set_role(resources.get().begin()->role());
}
```


- Joris Van Remoortere


On Nov. 12, 2015, 10:26 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 12, 2015, 10:26 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 12, 2015, 3:21 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 11, 2015, 5:35 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 11, 2015, 8:13 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > One thing I think is not entirely clean is repetition of some validation 
> > checks. For example, you check whether a role is set twice: while 
> > constructing a `QuotaInfo` instance and while validating it. I think we can 
> > sacrifice one check, for example in the `createQuotaInfo()` function, and 
> > leave a comment there that an object should be validated afterwards. 
> > Alternatively, we can do validation inside `createQuotaInfo()`, but this 
> > seems inconsistent to the way other endpoint handlers operate (e.g. 
> > maintenance, dynamic reservations).

Unfortunately, we need both checks.
a) as validate quotaInfo is a general check for quotaInfo we should validate it 
there
b) in createQuotaInfo I need to extract the role, therefore I need to verify 
that there is at least one resource


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota.cpp, line 81
> > 
> >
> > Any reason why you switch to "must" from "may" here?

Because before it was about stuff which is forbidden, this is about stuff which 
is required


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota.cpp, line 84
> > 
> >
> > Do we prohibit empty roles?

After discussion with Joris yes.


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 148
> > 
> >
> > You call it "request query string" above, any reason you change the 
> > name?

I considered both, the reason for this choice:
Above it is about stuff inside the query string, the last two checks are more 
concerned with the request itself (being for role). The alternative would have 
been something like "Quota request query string (...) including resource with 
an unknown role ".." which I felt not really parsable...


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 155
> > 
> >
> > Do you want to phrase it similar to error messages above? Something 
> > like: "Failed to fulfill quota set request ('...') for a role with already 
> > set quota"?

I used Failed if some other operation failed, this is more like "Missing 
'resources' in set quota request query string".
If you want I can change this to "Unable to fullfill quota set request"


- Joerg


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


On Nov. 11, 2015, 5:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 11, 2015, 5:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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


I have troubles seeing the whole patch. Maybe it's because a circular 
dependency sneaked in? Could you please remvoe it?


src/master/quota.cpp (lines 29 - 30)


Is there a newline inbetween?



src/master/quota_handler.cpp (line 83)


Remove extra newline



src/master/quota_handler.cpp (line 154)


Indentation is not consistent to the rest of the patch.


- Alexander Rukletsov


On Nov. 11, 2015, 5:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 11, 2015, 5:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 148
> > 
> >
> > You call it "request query string" above, any reason you change the 
> > name?
> 
> Joerg Schad wrote:
> I considered both, the reason for this choice:
> Above it is about stuff inside the query string, the last two checks are 
> more concerned with the request itself (being for role). The alternative 
> would have been something like "Quota request query string (...) including 
> resource with an unknown role ".." which I felt not really parsable...

Actually made the messages more consistent.


> On Nov. 11, 2015, 6:48 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 155
> > 
> >
> > Do you want to phrase it similar to error messages above? Something 
> > like: "Failed to fulfill quota set request ('...') for a role with already 
> > set quota"?
> 
> Joerg Schad wrote:
> I used Failed if some other operation failed, this is more like "Missing 
> 'resources' in set quota request query string".
> If you want I can change this to "Unable to fullfill quota set 
> request"

Actually made the messages more consistent.


- Joerg


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


On Nov. 11, 2015, 8:13 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 11, 2015, 8:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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


One thing I think is not entirely clean is repetition of some validation 
checks. For example, you check whether a role is set twice: while constructing 
a `QuotaInfo` instance and while validating it. I think we can sacrifice one 
check, for example in the `createQuotaInfo()` function, and leave a comment 
there that an object should be validated afterwards. Alternatively, we can do 
validation inside `createQuotaInfo()`, but this seems inconsistent to the way 
other endpoint handlers operate (e.g. maintenance, dynamic reservations).


src/master/quota.cpp (line 78)


Any reason why you switch to "must" from "may" here?



src/master/quota.cpp (line 81)


Do we prohibit empty roles?



src/master/quota_handler.cpp (line 146)


You call it "request query string" above, any reason you change the name?



src/master/quota_handler.cpp (line 153)


Do you want to phrase it similar to error messages above? Something like: 
"Failed to fulfill quota set request ('...') for a role with already set quota"?


- Alexander Rukletsov


On Nov. 11, 2015, 5:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 11, 2015, 5:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Vinod Kone]

joerg, can you please make sure these reviews have linear dependencies? i'm
seeing some reviews that block multiple reviews!? are you not using the
post-reviews script?

On Tue, Nov 10, 2015 at 10:20 AM, Joerg Schad  wrote:

>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
>
> (Updated Nov. 10, 2015, 6:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris
> Van Remoortere.
>
>
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added Quota Request Validation.
>
>
> Diffs (updated)
> -
>
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
>   src/master/quota.hpp PRE-CREATION
>   src/master/quota.cpp PRE-CREATION
>   src/master/quota_handler.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/39285/diff/
>
>
> Testing
> ---
>
> make test
>
>
> Thanks,
>
> Joerg Schad
>
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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



src/master/quota.cpp (line 36)


Should we add a blank line here?
Code base seems inconsistent about it...


- Joerg Schad


On Nov. 10, 2015, 6:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 10, 2015, 6:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 6:10 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 6:20 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joseph Wu]

The tree structure of the reviews is intentional (at the moment).
It's currently roughly split into components like:

  / Registrar   / POST
Protobufs - /quota endpoint - Authentication
  \ Allocator \ \ GET
   \- DELETE

~Joseph

On Tue, Nov 10, 2015 at 10:25 AM, Vinod Kone  wrote:

> joerg, can you please make sure these reviews have linear dependencies? i'm
> seeing some reviews that block multiple reviews!? are you not using the
> post-reviews script?
>
> On Tue, Nov 10, 2015 at 10:20 AM, Joerg Schad  wrote:
>
> >
> > ---
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/39285/
> > ---
> >
> > (Updated Nov. 10, 2015, 6:20 p.m.)
> >
> >
> > Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris
> > Van Remoortere.
> >
> >
> > Bugs: MESOS-3199
> > https://issues.apache.org/jira/browse/MESOS-3199
> >
> >
> > Repository: mesos
> >
> >
> > Description
> > ---
> >
> > Added Quota Request Validation.
> >
> >
> > Diffs (updated)
> > -
> >
> >   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2
> >   src/master/quota.hpp PRE-CREATION
> >   src/master/quota.cpp PRE-CREATION
> >   src/master/quota_handler.cpp PRE-CREATION
> >
> > Diff: https://reviews.apache.org/r/39285/diff/
> >
> >
> > Testing
> > ---
> >
> > make test
> >
> >
> > Thanks,
> >
> > Joerg Schad
> >
> >
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 8:37 a.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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



src/master/quota_handler.cpp (line 75)


Add line



src/master/quota_handler.cpp (line 132)


Add line



src/master/quota_handler.cpp (line 146)


add 2 indents



src/master/quota_handler.cpp (line 149)


backticks



src/master/quota_handler.cpp (line 179)


roles



src/master/quota_handler.cpp (line 207)


validate



src/master/quota_handler.cpp (line 214)


`QuotaInfo`



src/master/quota_handler.cpp (line 218)


new line



src/master/quota_handler.cpp (line 223)


`QuotaInfo` quota request


- Joerg Schad


On Nov. 9, 2015, 11:30 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 9, 2015, 11:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 9:07 a.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 10 a.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 10, 2015, 2:03 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota.hpp PRE-CREATION 
  src/master/quota.cpp PRE-CREATION 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joris Van Remoortere]

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



src/master/master.hpp (line 876)


Why is this protected?



src/master/quota.hpp (lines 35 - 38)


let's indent the list.



src/master/quota.cpp (lines 36 - 37)


Why are these inside the namespaces as opposed to above?



src/master/quota.cpp (line 43)


shouldn't this check that there is at least 1 resource?



src/master/quota.cpp (line 49)


It's not necessarily a "request" at this point right? It's an arbitrary 
`QuotaInfo`?
Same below



src/master/quota.cpp (line 70)


s/Check all/Check that all/



src/master/quota.cpp (lines 72 - 83)


your previous error messages specified what the requirement was. These 
specify what is wrong. Please stay consistent.



src/master/quota.cpp (line 78)


new line



src/master/quota.cpp (line 81)


trailing whitespace



src/master/quota.cpp (line 84)


What do you think about a space after the comma?



src/master/quota_handler.cpp (lines 52 - 53)


Should we call this something like `createQuotaInfo` to math other similar 
factory functions?



src/master/quota_handler.cpp (line 55)


specifically: it's being constructed from json here.
Might be valuable to log the request, if we bother to log the action?



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


How come we don't leverage the validation routine you introduced in this 
patch here?



src/master/quota_handler.cpp (line 100)


Expected, followed by actual



src/master/quota_handler.cpp (line 106)


should we provide the query string that we couldn't decode?
Same for error messages below.



src/master/quota_handler.cpp (line 111)


Why the hashmap `Option get()` pattern versus `contains(key)`?



src/master/quota_handler.cpp (line 116)


same here regarding the get pattern question



src/master/quota_handler.cpp (lines 122 - 126)


If we call it `createQuotaInfo` then we can stay consistent in the 
terminology (for example here we're already using extract and convert to talk 
about the same action)



src/master/quota_handler.cpp (line 132)


Here you reference the kind of request (i.e. `set quota`). In the other 
messages you don't. Let's stay consistent. I actually like referencing the 
request type like you did in this error message :-)



src/master/quota_handler.cpp (line 145)


s/Check we/Check that we/


- Joris Van Remoortere


On Nov. 10, 2015, 6:20 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 10, 2015, 6:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
>   src/master/quota.hpp PRE-CREATION 
>   src/master/quota.cpp PRE-CREATION 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Klaus Ma]

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

Ship it!


Ship It!

- Klaus Ma


On Nov. 9, 2015, 8:51 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 9, 2015, 8:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Klaus Ma]

> On Nov. 7, 2015, 12:19 a.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 140
> > 
> >
> > Suggest to move it into the loop; if any role is not known by master, 
> > we did not need to continue to check others.
> 
> Alexander Rukletsov wrote:
> I think the flow is more readable how it's now: in the loop we 
> reconstruct the "reference" role, afterwards we check whether it is known to 
> the master. Also, my gut feeling is that typos in roles will not be that 
> frequent, so checking it once instead of per resource makes sense to me.
> 
> Joerg Schad wrote:
> There should just be a single role per request, why should I check that 
> in the loop?

`::protobuf::parse()` will be heavy, we can end loop early if the role is not 
known by master. But as @AlexR said, typos in role is rare case, it also make 
sense to me to keep current behavior.


> On Nov. 7, 2015, 12:19 a.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 180
> > 
> >
> > Should we move it into `validateQuotaRequest`? If any role is exist in 
> > master, we did not need to continue to check others. And in QuotaHandler, 
> > we had the pointer to master `Master* master`.
> 
> Alexander Rukletsov wrote:
> I'd keep it here, because it's related to how we currently process the 
> request, rather than whether the request is valid.
> 
> Joerg Schad wrote:
> There was an earlier comment from Joris mentioning that this isn't really 
> part of validatating the request (as it also involves state of the master).

Agree :).


- Klaus


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


On Nov. 9, 2015, 8:51 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 9, 2015, 8:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 9, 2015, 12:51 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs
-

  src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing (updated)
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 9, 2015, 7:31 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

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


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 9, 2015, 11:30 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make test


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 9, 2015, 8:16 a.m.)


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


Changes
---

Adressed comments.


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 119
> > 
> >
> > Should we also check whether `resource.get().role()` is empty? There 
> > should be the case that assign empty string to `resource.role`.

Do we explicitly prohibit empty roles?


> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 140
> > 
> >
> > Suggest to move it into the loop; if any role is not known by master, 
> > we did not need to continue to check others.

I think the flow is more readable how it's now: in the loop we reconstruct the 
"reference" role, afterwards we check whether it is known to the master. Also, 
my gut feeling is that typos in roles will not be that frequent, so checking it 
once instead of per resource makes sense to me.


- Alexander


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


On Nov. 6, 2015, 3:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 180
> > 
> >
> > Should we move it into `validateQuotaRequest`? If any role is exist in 
> > master, we did not need to continue to check others. And in QuotaHandler, 
> > we had the pointer to master `Master* master`.

I'd keep it here, because it's related to how we currently process the request, 
rather than whether the request is valid.


- Alexander


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


On Nov. 6, 2015, 3:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 180
> > 
> >
> > Should we move it into `validateQuotaRequest`? If any role is exist in 
> > master, we did not need to continue to check others. And in QuotaHandler, 
> > we had the pointer to master `Master* master`.
> 
> Alexander Rukletsov wrote:
> I'd keep it here, because it's related to how we currently process the 
> request, rather than whether the request is valid.

There was an earlier comment from Joris mentioning that this isn't really part 
of validatating the request (as it also involves state of the master).


> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 140
> > 
> >
> > Suggest to move it into the loop; if any role is not known by master, 
> > we did not need to continue to check others.
> 
> Alexander Rukletsov wrote:
> I think the flow is more readable how it's now: in the loop we 
> reconstruct the "reference" role, afterwards we check whether it is known to 
> the master. Also, my gut feeling is that typos in roles will not be that 
> frequent, so checking it once instead of per resource makes sense to me.

There should just be a single role per request, why should I check that in the 
loop?


- Joerg


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


On Nov. 6, 2015, 3:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Nov. 5, 2015, 9:59 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 102-110
> > 
> >
> > What do you think about using helpers from 
> > `include/mesos/resources.hpp`?  A bunch of these checks could be done on a 
> > `Resources` (plural) object.

I considered them but decided against them for the following reasons
- I really jsut want to check whether such additional field has been specified, 
consider for example 'isPersistentVolume' which also checks for persistence. 
Hence I would need to chech .has_disk() anyhow.
- Others seem like very trivial wrappers (e.g. isRevocable -> return 
resource.has_revocable()) in which case I would like to have one consistent way 
across all checks (which due the above point is checking the protobug fields 
without the helpers).


- Joerg


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


On Oct. 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Nov. 3, 2015, 1:37 p.m., Qian Zhang wrote:
> > src/master/quota_handler.cpp, line 123
> > 
> >
> > Suggest to change ```if``` to ```else if```, in this way, for the first 
> > resource, we do not need to compare it with itself.

I personally find the non else version easier to read as it is just a sequence 
of conditions being checked, but changed it anyhow.


- Joerg


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


On Oct. 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Oct. 25, 2015, 2:45 a.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 115
> > 
> >
> > It said the role maybe not set in allocator interface's comments 
> > (`setQuota`); so any case that the role passed validation in master but 
> > it's  still empty to allocator?

If I picked the correct comment ("n allocator implementation may assume quota 
for the given role is not set prior to the call and react accordingly if this 
assumption is violated (i.e. fail)").
This comment refers to whether a role must be exixting prior to the call, this 
check is concerned whether the request has a role set which currently is 
required.


- Joerg


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


On Oct. 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Nov. 6, 2015, 3:23 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Klaus Ma]

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



src/master/quota_handler.cpp (line 113)


Should we also check whether `resource.get().role()` is empty? There should 
be the case that assign empty string to `resource.role`.



src/master/quota_handler.cpp (line 124)


I'd suggest to move it before checking role to make all simple check 
together, i.e. `has_disk`, `has_revocable`.



src/master/quota_handler.cpp (line 134)


Suggest to move it into the loop; if any role is not known by master, we 
did not need to continue to check others.



src/master/quota_handler.cpp (line 174)


Should we move it into `validateQuotaRequest`? If any role is exist in 
master, we did not need to continue to check others. And in QuotaHandler, we 
had the pointer to master `Master* master`.


- Klaus Ma


On Nov. 6, 2015, 11:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 11:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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



src/master/quota_handler.cpp (line 118)


} else if


- Joerg Schad


On Nov. 6, 2015, 3:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joseph Wu]

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



src/master/quota_handler.cpp (lines 93 - 101)


What do you think about using helpers from `include/mesos/resources.hpp`?  
A bunch of these checks could be done on a `Resources` (plural) object.



src/master/quota_handler.cpp (lines 151 - 152)


Can you comment that this is guaranteed by the master?


- Joseph Wu


On Oct. 24, 2015, 12:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 24, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joseph Wu]

> On Nov. 2, 2015, 8:43 a.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 148-149
> > 
> >
> > As per @joris' comment in the previous review, should we check for 
> > errors after `.get()` calls?

I agree.  You're effectively double-converting JSON->Protobuf.

Note that you can put `google::protobuf::RepeatedPtrField` in a `foreach`.


- Joseph


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


On Oct. 24, 2015, 12:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 24, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Qian Zhang]

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



src/master/quota_handler.cpp (line 113)


Suggest to change ```if``` to ```else if```, in this way, for the first 
resource, we do not need to compare it with itself.


- Qian Zhang


On Oct. 25, 2015, 3:42 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 25, 2015, 3:42 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Oct. 25, 2015, 2:45 a.m., Klaus Ma wrote:
> > src/master/master.hpp, line 864
> > 
> >
> > Should we return Bad Request (404) for now, because we did not 
> > implement it yet.

As it is implemented in the following patches it does not matter too much in my 
opinion...


> On Oct. 25, 2015, 2:45 a.m., Klaus Ma wrote:
> > src/master/master.hpp, line 875
> > 
> >
> > Same as above.

see above...


- Joerg


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


On Oct. 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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

Ship it!


Looks good! The only thing I'm not sure about is how to group related checks in 
the best way. Do you think it makes sense to create one scope per group and 
attach related comment to the scope?


src/master/quota_handler.cpp (line 75)


Mind adding a comment this is a "reference" role which is deduced from the 
resources in the request and is not specified directly? For example:

```
// A role for which quota request is sent. Currently only one role per 
request is allowed. Since an operator does not specify role explicitly, it is 
deduced from the provided resources.
```



src/master/quota_handler.cpp (line 121)


s/including/may not include

for consistency with earlier messages



src/master/quota_handler.cpp (line 125)


Typo: known



src/master/quota_handler.cpp (line 133)


Backtick `QuotaInfo`?



src/master/quota_handler.cpp (lines 135 - 136)


As per @joris' comment in the previous review, should we check for errors 
after `.get()` calls?



src/master/quota_handler.cpp (lines 157 - 158)


I think it makes sense not only to mention validation has failed, but also 
specify where we are coming from into validate (set quota).

In that vein, I suggest to rephrase as follows:
"Failed to validate set quota request: " + `quotaInfo.error()`, both for 
response and for logging.


- Alexander Rukletsov


On Oct. 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Guangya Liu]

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



src/master/quota_handler.cpp (lines 157 - 158)


one line?


- Guangya Liu


On 十月 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated 十月 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Guangya Liu]

> On 十一月 2, 2015, 4:43 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 84
> > 
> >
> > Mind adding a comment this is a "reference" role which is deduced from 
> > the resources in the request and is not specified directly? For example:
> > 
> > ```
> > // A role for which quota request is sent. Currently only one role per 
> > request is allowed. Since an operator does not specify role explicitly, it 
> > is deduced from the provided resources.
> > ```

Alex, I think that we should treat the role as explicitly specified in the http 
request as the http request does include roles for the quota. The operator does 
specify the role explicitly in the http request, comments?


- Guangya


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


On 十月 24, 2015, 7:42 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated 十月 24, 2015, 7:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Oct. 24, 2015, 7:42 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs
-

  src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Klaus Ma]

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



src/master/master.hpp (line 864)


Should we return Bad Request (404) for now, because we did not implement it 
yet.



src/master/master.hpp (line 875)


Same as above.



src/master/quota_handler.cpp (line 105)


It said the role maybe not set in allocator interface's comments 
(`setQuota`); so any case that the role passed validation in master but it's  
still empty to allocator?


- Klaus Ma


On Oct. 25, 2015, 3:42 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 25, 2015, 3:42 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Oct. 22, 2015, 10:25 a.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 71-72
> > 
> >
> > How about initializing `role` with QuotaInfo.role? Or we agreed not to 
> > send role as part of the JSON request?

According to the current design doc no.


- Joerg


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


On Oct. 22, 2015, 4:38 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 22, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Oct. 23, 2015, 4:52 p.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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



src/master/master.hpp (lines 883 - 884)


Should this function be public, or protected is good enough?


- Alexander Rukletsov


On Oct. 22, 2015, 4:38 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 22, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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



src/master/master.hpp (line 878)


s/Check/Checks
s/Request/request

Do we capitalize "quota"?



src/master/master.hpp (lines 879 - 882)


Let's document more:
- Role is known to the master.
- Irrelevant fields in `Resources` are not set (i.e. reservation, disk).



src/master/master.hpp (lines 1976 - 1985)


Either comment or bookkeeping should go away. As per comment, do you think 
we need `quotaInfo` here now? It does not seem you use this variable in this 
RR. I'd suggest to kill `quotaInfo` here.



src/master/quota_handler.cpp (lines 51 - 52)


This does not apply any more, right?



src/master/quota_handler.cpp (line 60)


I think we should move this check to `Master::QuotaHandler::set()` and even 
convert to `CHECK_` because it's an internal invarian at this moment. It would 
be nice to reuse this function for other quota requests, means we do not need 
any checks here.



src/master/quota_handler.cpp (line 61)


I think we should agree on terminology: "quota request", "Quota Request", 
or "QuotaRequest".

We should be consistent here and everywhere (also in other RRs).



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


This fits in one line.



src/master/quota_handler.cpp (lines 71 - 72)


How about initializing `role` with QuotaInfo.role? Or we agreed not to send 
role as part of the JSON request?



src/master/quota_handler.cpp (line 88)


Let's be consistent and use present simple everywhere : ).

s/Checking that Resource does not contain non-relevant fields./Check that 
Resources does not contain fields non-relevant for quota.



src/master/quota_handler.cpp (line 89)


I think we can kill this comment.



src/master/quota_handler.cpp (line 93)


And this one.



src/master/quota_handler.cpp (line 97)


And this one.



src/master/quota_handler.cpp (line 102)


The code does something different to what the comment says : ).



src/master/quota_handler.cpp (line 115)


s/all resources are scalar/the resource is scalar



src/master/quota_handler.cpp (line 118)


This sounds incomplete to me. Maybe "includes" is better?



src/master/quota_handler.cpp (line 127)


We are not consistent in BadRequet error messages, but how about 
"Attempting to set quota for an unknown role ''"?



src/master/quota_handler.cpp (line 130)


s/Quota Info/QuotaInfo



src/master/quota_handler.cpp (lines 156 - 160)


This is a bug: not in sync with the way we store quotas:
```
master->quotas[quota.role()] = quota;
```.



src/master/quota_handler.cpp (line 159)


We do not support update currently, so the message is misleading. How about 
"Quota cannot be set for a role that already has quota"?



src/master/quota_handler.cpp (line 183)


Why this newline?


- Alexander Rukletsov


On Oct. 22, 2015, 4:38 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 22, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

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



src/master/quota_handler.cpp (line 162)


We don't need this comment any more, right?


- Alexander Rukletsov


On Oct. 22, 2015, 4:38 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 22, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Oct. 22, 2015, 10:57 a.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, lines 64-66
> > 
> >
> > This is an API change which contradicts the API described in the design 
> > doc and differs from, for example, dynamic reservations. Mind elaborating 
> > why you have opted for this change?

No strong preference, will switch back.


- Joerg


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


On Oct. 22, 2015, 4:38 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 22, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Oct. 22, 2015, 10:25 a.m., Alexander Rukletsov wrote:
> > src/master/master.hpp, line 878
> > 
> >
> > s/Check/Checks
> > s/Request/request
> > 
> > Do we capitalize "quota"?

I did, will adjust to you style


- Joerg


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


On Oct. 22, 2015, 4:38 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Oct. 22, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Oct. 22, 2015, 4:38 a.m.)


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


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Guangya Liu]

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



src/master/quota_handler.cpp (lines 29 - 31)


adjust the order of the two imports



src/master/quota_handler.cpp (line 57)


Can we put the "request" info to the log message for better debugability?



src/master/quota_handler.cpp (line 111)


Can we put those two roles in the Error msg?



src/master/quota_handler.cpp (line 126)


Can we use 
return Error("Quota Request for unknown role '" + role + "'");



src/master/quota_handler.cpp (line 182)


Remove this new line


- Guangya Liu


On 十月 19, 2015, 8:11 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated 十月 19, 2015, 8:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Oct. 19, 2015, 8:11 p.m.)


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


Changes
---

Adressed comments


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

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

(Updated Oct. 15, 2015, 6:50 p.m.)


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


Changes
---

Rebased


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


Repository: mesos


Description
---

Added Quota Request Validation.


Diffs (updated)
-

  src/master/master.hpp e7b16fdd21a8caa77a39956a8520cf1381186598 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---


Thanks,

Joerg Schad