mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Re: Review Request 60253: Placed the `convertResourceFormat` calls after resource validation.
Date Wed, 21 Jun 2017 00:56:15 GMT

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


Ship it!




These particular fixes seem fine (and I couldn't spot any places we are neglecting to get
validation/conversion wrong, from a quick look), but the current approach to input validation
and conversion seems very error-prone to me. We need to separately:

(a) get one or more `Resource` objects (e.g., via `protobuf::parse<Resource>`)
(b) validate that the `Resource` is valid
(c) convert `Resource` to post-refinement format
(d) convert/aggregate `Resource` into `Resources`
(e) in many cases, add the `Resources` into an operation, which is itself validated for a
different set of criteria.

Doing all 5 things at each call-site seems regrettable. For example, we could consider combining
validation and format conversion into a single helper; or perhaps have an "try adding `Resource`
to `Resources`" that validates the `Resource`, converts it to the right format, and then either
returns the updated `Resources` or an error if validation.

- Neil Conway


On June 21, 2017, 12:47 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60253/
> -----------------------------------------------------------
> 
> (Updated June 21, 2017, 12:47 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `Resources` requires that the `Resource` objects being constructed with
> be validated and are in the "post-reservation-refinement" format.
> The `convertResourceFormat` was previously placed after the validation
> of operations, but we construct `Resources` prior to operation
> validation. This patch moves the conversion logic earlier to be done
> right after resource validation.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 801b80933985a95d58f6b3b9973558d0c5a4410e 
> 
> 
> Diff: https://reviews.apache.org/r/60253/diff/2/
> 
> 
> Testing
> -------
> 
> ```bash
> curl -i -d slaveId=9091bf1d-58cb-4f54-b804-fe3eef85facd-S0 -d resources='[{"name":"cpus","type":"SCALAR","scalar":{"value":1},"role":"ads","reservation":{}},{"name":"mem","type":"SCALAR","scalar":{"value":1024},"role":"ads","reservation":{}}]'
-X POST http://127.0.0.1:5050/master/reserve
> ```
> 
> 
> Thanks,
> 
> Michael Park
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message