----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60563/#review179458 -----------------------------------------------------------
Fix it, then Ship it! src/common/resources_utils.hpp Lines 135-139 (original), 135-139 (patched) <https://reviews.apache.org/r/60563/#comment254158> Can you add a comment about the reasoning on why we allow a (pre or post) framework to use any format? Seems pretty suprising that we would allow pre frameworks using post format, pre frameworks using endpoint format, post frameworks using pre format, etc. Also, some cases seem to need to be prevented at some layer. For example, if a framework doesn't have the reservation refinement capability, we shouldn't let them create refined reservations, because they won't be able to see them after they create them due to our capability filtering of offers. Do we prevent pre capability frameworks from making refined reservations? Not sure if you want to address this in this review as part of this validation, or as a separate ticket? Can you have that as a blocker for 1.4? src/common/resources_utils.cpp Lines 215 (patched) <https://reviews.apache.org/r/60563/#comment254160> This will set the 'reserve' field if it was previously unset. Seems unexpected by the caller? It seems injectAllocationInfo has this same problem (I thought I had avoided this intentionally, but I guess not or it was lost...). It's just unfortunate from an error message perspective, since rather than telling the framework that they forgot to set the 'reserve' field, we tell them that they are providing empty resources. src/common/resources_utils.cpp Lines 231 (patched) <https://reviews.apache.org/r/60563/#comment254161> Ditto here for accidentally setting the field. src/common/resources_utils.cpp Lines 247 (patched) <https://reviews.apache.org/r/60563/#comment254162> Ditto here for accidentally setting the field. src/common/resources_utils.cpp Lines 263 (patched) <https://reviews.apache.org/r/60563/#comment254163> Ditto here for accidentally setting the field. src/common/resources_utils.cpp Lines 288 (patched) <https://reviews.apache.org/r/60563/#comment254164> Ditto here for accidentally setting the field. src/common/resources_utils.cpp Lines 303-304 (patched) <https://reviews.apache.org/r/60563/#comment254165> Ditto here for accidentally setting the field. src/common/resources_utils.cpp Lines 340 (patched) <https://reviews.apache.org/r/60563/#comment254166> Ditto here for accidentally setting the field. src/common/resources_utils.cpp Lines 354-356 (patched) <https://reviews.apache.org/r/60563/#comment254167> Hm.. is it the responsibility of this function to return an error for unknown operations? Curious how that fits into the logic in the master. - Benjamin Mahler On July 1, 2017, 12:56 a.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60563/ > ----------------------------------------------------------- > > (Updated July 1, 2017, 12:56 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-7735 > https://issues.apache.org/jira/browse/MESOS-7735 > > > Repository: mesos > > > Description > ------- > > Updated `validateAndUpgradeResources` to operate on `Operation`s. > > > Diffs > ----- > > src/common/resources_utils.hpp 7128297c45fbf94af9384b678bf201f3d9c48b65 > src/common/resources_utils.cpp 3a6a57817d4b0c4f4133b0a8d2b6f4d9ea31ea6b > > > Diff: https://reviews.apache.org/r/60563/diff/2/ > > > Testing > ------- > > > Thanks, > > Michael Park > >
