mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 42471: Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
Date Thu, 21 Jan 2016 17:50:28 GMT

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



src/common/resources.cpp (lines 102 - 108)
<https://reviews.apache.org/r/42471/#comment176646>

    Can you do the following to be more consistent with others
    ```
    if (left.has_path() && left.path() != right.path()) {
      return false;
    }
    if (left.has_mount() && left.mount() != right.mount()) {
      return false;
    }
    ```



src/common/resources.cpp (lines 251 - 255)
<https://reviews.apache.org/r/42471/#comment176642>

    I'd prefer removing this CHECK here since it's on the critical path of resource calculation.



src/common/resources.cpp (lines 309 - 328)
<https://reviews.apache.org/r/42471/#comment176643>

    Ditto on merge them into a single if clause.



src/common/resources.cpp (lines 315 - 317)
<https://reviews.apache.org/r/42471/#comment176644>

    Ditto on removing this CHECK.



src/common/resources.cpp (lines 319 - 322)
<https://reviews.apache.org/r/42471/#comment176645>

    This does not look correct. Two identical MOUNT disk resource should be subtractable.



src/v1/resources.cpp (lines 102 - 104)
<https://reviews.apache.org/r/42471/#comment176636>

    Can you do the following to be consistent with others:
    ```
    if (left.has_path() && left.path() != right.path()) {
      return false;
    }
    
    if (left.has_mount() && left.mount() != right.mount()) {
      return false;
    }
    
    return true;
    ```



src/v1/resources.cpp (lines 247 - 268)
<https://reviews.apache.org/r/42471/#comment176638>

    We should merge those check under the same `has_disk` if clause:
    
    ```
    if (left.has_disk()) {
      // Check if disk() are equal
      // MOUNT check
      // persistence check
    }
    ```



src/v1/resources.cpp (lines 253 - 255)
<https://reviews.apache.org/r/42471/#comment176647>

    Ditto on removing it.



src/v1/resources.cpp (lines 315 - 317)
<https://reviews.apache.org/r/42471/#comment176648>

    Ditto on removing it.



src/v1/resources.cpp (lines 319 - 322)
<https://reviews.apache.org/r/42471/#comment176649>

    Ditto. Please adjust the logics here according to my comments above.


- Jie Yu


On Jan. 21, 2016, 7:36 a.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42471/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2016, 7:36 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4380
>     https://issues.apache.org/jira/browse/MESOS-4380
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Multiple Disk: Adjusted resource arithmetic for 'DiskInfo.Source'.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp efe2b1de0c277db62d7f7cc5ff1cd9143b9f632a 
>   include/mesos/v1/mesos.hpp 961042d8e4944a475076b829966020d62175d726 
>   src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a 
>   src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc 
> 
> Diff: https://reviews.apache.org/r/42471/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>


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