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 60017: Resources: Updated utilities.
Date Tue, 13 Jun 2017 19:55:00 GMT

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




src/common/resources.cpp
Line 1378 (original), 1379-1384 (patched)
<https://reviews.apache.org/r/60017/#comment251513>

    Hm.. it's not obvious to me if this is the right thing to do, how much thought did you
put into this? (can't tell given there's no comment)
    
    i.e. Should we be reducing the reservations down to a single one (and do we care about
the distinction between static and dynamic for this stripped scalar quantity?) vs stripping
metadata but preserving the stack.
    
    I think before we just cared about which role it was reserved to, not whether it was static
or dynamic, so this is a change to the behavior.



src/common/resources.cpp
Lines 2055-2074 (original)
<https://reviews.apache.org/r/60017/#comment251509>

    This seems unrelated, can you pull it out of this change so that the diff is as clean
as possible?



src/common/resources.cpp
Lines 2100-2115 (original), 2085-2101 (patched)
<https://reviews.apache.org/r/60017/#comment251514>

    Can you pull the printing out into a separate patch and show in the description what the
format looks like?



src/common/resources_utils.cpp
Lines 42-44 (original), 42-44 (patched)
<https://reviews.apache.org/r/60017/#comment251510>

    Wow I didn't realize this function existed, looks really brittle. Hopefully whenever we
write integration tests for new operations it doesn't work unless this is updated. :)



src/common/resources_utils.cpp
Lines 56-57 (original), 56-60 (patched)
<https://reviews.apache.org/r/60017/#comment251511>

    A little comment here would be great, took me a bit of time to understand what this was
doing.



src/common/resources_utils.cpp
Lines 63-68 (original), 66-71 (patched)
<https://reviews.apache.org/r/60017/#comment251512>

    Not yours, but I wish there was a little comment here about why source is left in (took
me a bit of time to figure that out)


- Benjamin Mahler


On June 13, 2017, 8:35 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60017/
> -----------------------------------------------------------
> 
> (Updated June 13, 2017, 8:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
>     https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Resources: Updated utilities.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/common/resources_utils.cpp c3088433d8c147b06dbef6f310fbe4059c3dc0ba 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60017/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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