mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 60563: Updated `validateAndUpgradeResources` to operate on `Operation`s.
Date Sat, 01 Jul 2017 06:31:08 GMT

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


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