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