Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Bernd Mathiske]

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



src/master/master.hpp (line 1978)


Suggestion: move this to the end of the struct, ca. line 1983 and change 
the comment to this.

"NOTE: There is no direct representation of quota information here to 
support that the operator can associate quota (by name) with a role before the 
role is created. Such ordering of operator requests prevents a race of 
premature unbounded allocation that setting quota first is intended to contain. 
The dynamic role/quota relation is administrated by the master instance."


- Bernd Mathiske


On Nov. 10, 2015, 8:19 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 10, 2015, 8:19 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c235209743de6e84deb44df31c61948f4dc8b8eb 
>   src/Makefile.am ae2740a5b56351d9fd82ae3bd5c733d10a90bf2f 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Nov. 5, 2015, 8:24 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 83-88
> > 
> >
> > Why don't you just parse a QuotaInfo object instead of a 
> > form-serialized body (with JSON components)?
> > 
> > The maintenance endpoints do this for simplicity.
> 
> Alexander Rukletsov wrote:
> Because we didn't want to expose the internal structure (`QuotaInfo`) to 
> the outside world. In the future we may want to allow operators to set quotas 
> for multiple roles in one call, which we can easier do if we do not tie the 
> operator API to `QuotaInfo` protobuf. Does it make sense?
> 
> Joseph Wu wrote:
> `QuotaInfo` should already be exposed, since it's in the `include` folder.
> 
> As for setting multiple `QuotaInfo`s in a single call, wouldn't that be 
> simpler to implement via a JSON array?
> (Maybe you can parse the request body as an array, but reject calls with 
> more than one quota request in the MVP.)
> 
> Alexander Rukletsov wrote:
> I should have been more precise and should have elaborated more in my 
> first reply. 
> 
> `QuotaInfo` is "mainly" the storage protobuf. It is indeed in the public 
> includes, but the reason for this is that it's part of the allocator 
> interface. Also we may want to let frameworks know what the quota of their 
> role is.
> 
> We do not aim to document the operator API for quota by exposing 
> `QuotaInfo` to them. We also do not intend to make the operator API for quota 
> 1:1 correspondent to `QuotaInfo` message. Let's think about post MVP: quota 
> may get optional chunks, limits; it may be set for a framework. I envision 
> the `QuotaInfo` message becomes a complex protobuf (think `Resource`). The 
> API for operators should stay simple and flexible, I am reluctant to force 
> operators provide a JSON view of `QuotaInfo` any time they want to set or 
> update quota. Recalling some of our discussions, we may even have different 
> endpoints for setting quota (`/master/quota`, `/master/roles/`)!
> 
> Does it make sense?
> 
> Joseph Wu wrote:
> That sounds similar to the motivation for having V1 protos (i.e. having a 
> public proto that gets converted into an internal-but-publically-exposed 
> object).  And even if the API expects a non-`QuotaInfo` object, it would 
> still be helpful to have a schema, of some sort.  Based on what's present in 
> the review, it could just be a wrapper around a `Resource` proto.
> 
> Side note: Apparently, the `Resource` object is a bad example.  MPark 
> found out yesterday that you can't take a `Resource` that Mesos outputs (via 
> `/state`) and pass it to an endpoint that takes `Resource` as an input.  You 
> have to reformat the `Resource`, even if both input/output are JSON formatted.

Having schema is helpful, I agree, but not always feasible. For example, if you 
hit the `/reserve` endpoint, there is neither schema nor corresponding protobuf 
message. An operator *just* provides resources. Similar for quota, an operator 
provides a collection of resources.


- Alexander


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


On Nov. 10, 2015, 4:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 10, 2015, 4:19 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c235209743de6e84deb44df31c61948f4dc8b8eb 
>   src/Makefile.am ae2740a5b56351d9fd82ae3bd5c733d10a90bf2f 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39211, 39018, 39102, 36913, 38059]

All tests passed.

- Mesos ReviewBot


On Nov. 10, 2015, 4:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 10, 2015, 4:19 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c235209743de6e84deb44df31c61948f4dc8b8eb 
>   src/Makefile.am ae2740a5b56351d9fd82ae3bd5c733d10a90bf2f 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39211, 39018, 39102, 36913, 38059]

All tests passed.

- Mesos ReviewBot


On Nov. 10, 2015, 4:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 10, 2015, 4:19 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c235209743de6e84deb44df31c61948f4dc8b8eb 
>   src/Makefile.am ae2740a5b56351d9fd82ae3bd5c733d10a90bf2f 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

(Updated Nov. 10, 2015, 4:19 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


Changes
---

Joris' comments.


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs (updated)
-

  src/CMakeLists.txt c235209743de6e84deb44df31c61948f4dc8b8eb 
  src/Makefile.am ae2740a5b56351d9fd82ae3bd5c733d10a90bf2f 
  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/38059/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Nov. 10, 2015, 7:26 a.m., Joris Van Remoortere wrote:
> > src/master/quota_handler.cpp, line 48
> > 
> >
> > Let's sync this to a `Try` to avoid confusion.

I think I'd better remove the whole function alltogether.


- Alexander


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


On Nov. 9, 2015, 11:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 11:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
>   src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Oct. 13, 2015, 3:35 p.m., Joris Van Remoortere wrote:
> > src/master/quota_handler.cpp, line 60
> > 
> >
> > Should we follow the pattern of other validation routines where we use 
> > the validation namespace and pull these utility routines into a separate 
> > file?
> > 
> > I think we can separate the validation of the request from the content 
> > of the protobuf. Syntax vs. semantic.
> > 
> > See maintenance as an example.
> 
> Alexander Rukletsov wrote:
> The reason it's conflated into one function is because in order to do 
> proper validation we should convert JSON to protobuf. Some checks are 
> performed with the JSON object and some with the converted protobuf. I though 
> that having three functions `validateJSON`, `convertToQuotaInfo`, and 
> `validateQuotaInfo` is too much.
> 
> Joseph Wu wrote:
> What specific validation are you doing to the JSON objects (other than 
> converting to Protobuf)?  And why can't the validation wait until after the 
> JSON->Protobuf conversion sufficient?
> 
> Alexander Rukletsov wrote:
> Let's discuss this in the next review 
> https://reviews.apache.org/r/39285/, where validation is actually implemented.

We will address it in the next review.


- Alexander


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


On Nov. 9, 2015, 11:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 11:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
>   src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Oct. 6, 2015, 12:40 p.m., Guangya Liu wrote:
> > src/master/quota_handler.cpp, line 60
> > 
> >
> > I think that this function is mainly doing getQuoteRequest()?
> 
> Alexander Rukletsov wrote:
> Converting JSON to protobuf is not a big deal, I wanted to put the accent 
> on the validation checks.
> 
> Guangya Liu wrote:
> Can we split this to two functions, one is validataQuotaRequest and the 
> other is getQuotaRequest?
> 
> Joerg Schad wrote:
> Constructing the protobuf is part of the basic checks and from my point 
> it feels ok that validates return the protobuf.

We revisited the design and I think we addressed this issue in the next review. 
I'll drop it here, please check the next patch in the chain and comment there 
in case you feel like.


- Alexander


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


On Nov. 9, 2015, 11:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 11:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
>   src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Nov. 5, 2015, 8:24 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 83-88
> > 
> >
> > Why don't you just parse a QuotaInfo object instead of a 
> > form-serialized body (with JSON components)?
> > 
> > The maintenance endpoints do this for simplicity.
> 
> Alexander Rukletsov wrote:
> Because we didn't want to expose the internal structure (`QuotaInfo`) to 
> the outside world. In the future we may want to allow operators to set quotas 
> for multiple roles in one call, which we can easier do if we do not tie the 
> operator API to `QuotaInfo` protobuf. Does it make sense?
> 
> Joseph Wu wrote:
> `QuotaInfo` should already be exposed, since it's in the `include` folder.
> 
> As for setting multiple `QuotaInfo`s in a single call, wouldn't that be 
> simpler to implement via a JSON array?
> (Maybe you can parse the request body as an array, but reject calls with 
> more than one quota request in the MVP.)

I should have been more precise and should have elaborated more in my first 
reply. 

`QuotaInfo` is "mainly" the storage protobuf. It is indeed in the public 
includes, but the reason for this is that it's part of the allocator interface. 
Also we may want to let frameworks know what the quota of their role is.

We do not aim to document the operator API for quota by exposing `QuotaInfo` to 
them. We also do not intend to make the operator API for quota 1:1 
correspondent to `QuotaInfo` message. Let's think about post MVP: quota may get 
optional chunks, limits; it may be set for a framework. I envision the 
`QuotaInfo` message becomes a complex protobuf (think `Resource`). The API for 
operators should stay simple and flexible, I am reluctant to force operators 
provide a JSON view of `QuotaInfo` any time they want to set or update quota. 
Recalling some of our discussions, we may even have different endpoints for 
setting quota (`/master/quota`, `/master/roles/`)!

Does it make sense?


- Alexander


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


On Nov. 9, 2015, 11:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 11:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
>   src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Joris Van Remoortere]

> On Oct. 6, 2015, 12:40 p.m., Guangya Liu wrote:
> > src/master/quota_handler.cpp, line 65
> > 
> >
> > Just as early comments: What about PUT and DELETE?
> 
> Alexander Rukletsov wrote:
> See my comment in the subsequent review: 
> https://reviews.apache.org/r/39285/diff/4?file=1103067#file1103067line60
> I think we should move this line out of this function.
> 
> Guangya Liu wrote:
> PUT is not MVP, DELETE should be still in MVP, right?
> 
> Alexander Rukletsov wrote:
> Yes.

We'll address Delete once we implement remove quota.


- Joris


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


On Nov. 9, 2015, 11:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 11:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
>   src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Oct. 7, 2015, 2:41 p.m., Bernd Mathiske wrote:
> > src/master/quota_handler.cpp, line 66
> > 
> >
> > Given that we have the request object, we can output more diagnostic 
> > info here.
> 
> Guangya Liu wrote:
> I think that we can use BadRequest("Expecting POST"); instead, please 
> refer to https://github.com/apache/mesos/blob/master/src/master/http.cpp#L686
> 
> Alexander Rukletsov wrote:
> We are using `BadRequest` in the caller. The intention is to send HTTP 
> responses only from the "entry" function, which processes the request 
> (`Master::QuotaHandler::request()`, will be renamed to 
> `Master::QuotaHandler::set()`). All callees return errors. Does it make sense?
> 
> Bernd Mathiske wrote:
> Yes, but the argument of Error() here could include what the request was, 
> so a human can inspect it in the log. Unless it is clear from the context 
> that the request has already been logged. But I prefer context-free extra 
> diagnostics :-)

We do provide more info, please look in https://reviews.apache.org/r/36913/. 
Also, this line is changed to a CHECK in the next review in the chain.


- Alexander


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


On Nov. 9, 2015, 11:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 11:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
>   src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Nov. 5, 2015, 8:24 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, line 52
> > 
> >
> > I feel this shouldn't be left for later.

I think I have to apologize for the confusion I create with this ticket. The 
implementation of this function is a stub, it is expanded (and documented) in 
the next patch by Jörg. The reason for this is poor job we did in splitting 
tasks between contributors.


> On Nov. 5, 2015, 8:24 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 70-74
> > 
> >
> > These are TODO's right?
> > 
> > Also, typos:
> > s/including:/including/
> > The third "Check" s/Check/check/
> > s/reservatin/reservation/

Please see above.


> On Nov. 5, 2015, 8:24 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 83-88
> > 
> >
> > Why don't you just parse a QuotaInfo object instead of a 
> > form-serialized body (with JSON components)?
> > 
> > The maintenance endpoints do this for simplicity.

Because we didn't want to expose the internal structure (`QuotaInfo`) to the 
outside world. In the future we may want to allow operators to set quotas for 
multiple roles in one call, which we can easier do if we do not tie the 
operator API to `QuotaInfo` protobuf. Does it make sense?


- Alexander


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


On Nov. 5, 2015, 7:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 5, 2015, 7:28 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Oct. 6, 2015, 12:40 p.m., Guangya Liu wrote:
> > src/master/quota_handler.cpp, line 65
> > 
> >
> > Just as early comments: What about PUT and DELETE?
> 
> Alexander Rukletsov wrote:
> See my comment in the subsequent review: 
> https://reviews.apache.org/r/39285/diff/4?file=1103067#file1103067line60
> I think we should move this line out of this function.
> 
> Guangya Liu wrote:
> PUT is not MVP, DELETE should be still in MVP, right?

Yes.


- Alexander


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


On Nov. 5, 2015, 7:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 5, 2015, 7:28 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Bernd Mathiske]

> On Oct. 7, 2015, 7:41 a.m., Bernd Mathiske wrote:
> > src/master/quota_handler.cpp, line 66
> > 
> >
> > Given that we have the request object, we can output more diagnostic 
> > info here.
> 
> Guangya Liu wrote:
> I think that we can use BadRequest("Expecting POST"); instead, please 
> refer to https://github.com/apache/mesos/blob/master/src/master/http.cpp#L686
> 
> Alexander Rukletsov wrote:
> We are using `BadRequest` in the caller. The intention is to send HTTP 
> responses only from the "entry" function, which processes the request 
> (`Master::QuotaHandler::request()`, will be renamed to 
> `Master::QuotaHandler::set()`). All callees return errors. Does it make sense?

Yes, but the argument of Error() here could include what the request was, so a 
human can inspect it in the log. Unless it is clear from the context that the 
request has already been logged. But I prefer context-free extra diagnostics :-)


- Bernd


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


On Nov. 5, 2015, 11:28 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 5, 2015, 11:28 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

> On Oct. 13, 2015, 3:35 p.m., Joris Van Remoortere wrote:
> > src/master/quota_handler.cpp, line 60
> > 
> >
> > Should we follow the pattern of other validation routines where we use 
> > the validation namespace and pull these utility routines into a separate 
> > file?
> > 
> > I think we can separate the validation of the request from the content 
> > of the protobuf. Syntax vs. semantic.
> > 
> > See maintenance as an example.
> 
> Alexander Rukletsov wrote:
> The reason it's conflated into one function is because in order to do 
> proper validation we should convert JSON to protobuf. Some checks are 
> performed with the JSON object and some with the converted protobuf. I though 
> that having three functions `validateJSON`, `convertToQuotaInfo`, and 
> `validateQuotaInfo` is too much.
> 
> Joseph Wu wrote:
> What specific validation are you doing to the JSON objects (other than 
> converting to Protobuf)?  And why can't the validation wait until after the 
> JSON->Protobuf conversion sufficient?

Let's discuss this in the next review https://reviews.apache.org/r/39285/, 
where validation is actually implemented.


- Alexander


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


On Nov. 5, 2015, 7:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 5, 2015, 7:28 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Bernd Mathiske]

> On Oct. 7, 2015, 7:41 a.m., Bernd Mathiske wrote:
> > src/master/quota_handler.cpp, line 74
> > 
> >
> > Again, we could output more info here.
> 
> Alexander Rukletsov wrote:
> What exactly?

I checked the error() method. It suffices.


- Bernd


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


On Nov. 5, 2015, 11:28 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 5, 2015, 11:28 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

(Updated Nov. 9, 2015, 4:39 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


Changes
---

Joris' comment.


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs (updated)
-

  src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
  src/Makefile.am 938b8d403024e7b705b6088384292ad80452d9c6 
  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/38059/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

(Updated Nov. 9, 2015, 3:59 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


Changes
---

Added a file for validation.


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs (updated)
-

  src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
  src/Makefile.am 938b8d403024e7b705b6088384292ad80452d9c6 
  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/38059/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Joseph Wu]

> On Nov. 5, 2015, 12:24 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 83-88
> > 
> >
> > Why don't you just parse a QuotaInfo object instead of a 
> > form-serialized body (with JSON components)?
> > 
> > The maintenance endpoints do this for simplicity.
> 
> Alexander Rukletsov wrote:
> Because we didn't want to expose the internal structure (`QuotaInfo`) to 
> the outside world. In the future we may want to allow operators to set quotas 
> for multiple roles in one call, which we can easier do if we do not tie the 
> operator API to `QuotaInfo` protobuf. Does it make sense?

`QuotaInfo` should already be exposed, since it's in the `include` folder.

As for setting multiple `QuotaInfo`s in a single call, wouldn't that be simpler 
to implement via a JSON array?
(Maybe you can parse the request body as an array, but reject calls with more 
than one quota request in the MVP.)


- Joseph


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


On Nov. 9, 2015, 8:39 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 8:39 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
>   src/Makefile.am 938b8d403024e7b705b6088384292ad80452d9c6 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Joris Van Remoortere]

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


Let's just try and make this chain a little easier to follow. Right now the 
reviews are heavily inter-leaved.


src/master/quota_handler.cpp (line 48)


Let's sync this to a `Try` to avoid confusion.



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


Let's just leave the comment here, since Joerg is implementing this 
validation. This will reduce the rebasing overhead.



src/master/quota_handler.cpp (line 69)


Let's just construct a dummy `QuotaInfo` here (like your did above) to 
avoid the intricate dependency between this and the next patch.



src/master/quota_handler.cpp (line 74)


We're prefacing what is bout to happen here, right? s/Populated/Populate/
s/We do it/We do this



src/master/quota_handler.cpp (line 79)


Can we initialize this as `quota = {quotaInfo}`?



src/master/quota_handler.cpp (line 82)


s/Update registry/Update the registry/


- Joris Van Remoortere


On Nov. 9, 2015, 11:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 9, 2015, 11:14 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
>   src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
>   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/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

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


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs (updated)
-

  src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
  src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
  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/38059/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

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


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs (updated)
-

  src/CMakeLists.txt f6ae05d4652df6de98a9e110efed87f7fcbd29f9 
  src/Makefile.am 6ec0488027d6cfccc63ac3a6a8b0c3d8eb6c3330 
  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/38059/diff/


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Joseph Wu]

> On Oct. 13, 2015, 8:35 a.m., Joris Van Remoortere wrote:
> > src/master/quota_handler.cpp, line 60
> > 
> >
> > Should we follow the pattern of other validation routines where we use 
> > the validation namespace and pull these utility routines into a separate 
> > file?
> > 
> > I think we can separate the validation of the request from the content 
> > of the protobuf. Syntax vs. semantic.
> > 
> > See maintenance as an example.
> 
> Alexander Rukletsov wrote:
> The reason it's conflated into one function is because in order to do 
> proper validation we should convert JSON to protobuf. Some checks are 
> performed with the JSON object and some with the converted protobuf. I though 
> that having three functions `validateJSON`, `convertToQuotaInfo`, and 
> `validateQuotaInfo` is too much.

What specific validation are you doing to the JSON objects (other than 
converting to Protobuf)?  And why can't the validation wait until after the 
JSON->Protobuf conversion sufficient?


- Joseph


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


On Nov. 5, 2015, 11:28 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 5, 2015, 11:28 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Joseph Wu]

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



src/master/quota_handler.cpp (line 52)


I feel this shouldn't be left for later.



src/master/quota_handler.cpp (lines 70 - 74)


These are TODO's right?

Also, typos:
s/including:/including/
The third "Check" s/Check/check/
s/reservatin/reservation/



src/master/quota_handler.cpp (lines 83 - 88)


Why don't you just parse a QuotaInfo object instead of a form-serialized 
body (with JSON components)?

The maintenance endpoints do this for simplicity.



src/master/quota_handler.cpp (line 118)


Typo: s/reuqest/request/


- Joseph Wu


On Nov. 5, 2015, 11:28 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38059/
> ---
> 
> (Updated Nov. 5, 2015, 11:28 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Processing quota request consists of several stages: request validation, 
> sanity check and so on. This patch creates a basic workflow for quota 
> requests, while the stages are implemented in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38059/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

(Updated Nov. 5, 2015, 2:05 p.m.)


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


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs (updated)
-

  src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38059: Quota: Created a bare pipeline for quota set requests.

[Alexander Rukletsov]

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

(Updated Nov. 3, 2015, 3:46 p.m.)


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


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


Repository: mesos


Description
---

Processing quota request consists of several stages: request validation, sanity 
check and so on. This patch creates a basic workflow for quota requests, while 
the stages are implemented in subsequent patches.


Diffs
-

  src/master/master.hpp 3aa7017287506938d77bbfb1e06f510101009ee3 
  src/master/quota_handler.cpp PRE-CREATION 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov