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


I would suggest to write a summarizing comment about how coalescing works, 
preferably with algorithmic complexity : ). This may help us to evaluate 
performance and maybe tune the algorithm later on when we have more usage 
experience and feedback from operators.


src/common/values.cpp (line 91)
<https://reviews.apache.org/r/38158/#comment154064>

    Mention it's an in-place operation, please



src/common/values.cpp (lines 92 - 93)
<https://reviews.apache.org/r/38158/#comment154066>

    s/is/are
    
    I would suggest we write high level comments. How about this: "We assume 
ranges are sorted in ascending order of the lower endpoint."



src/common/values.cpp (line 94)
<https://reviews.apache.org/r/38158/#comment154069>

    A high level comment.
    
    `RepeatedPtrField` is a linear data structure, removing elements from it 
causes relocating all elements beyond the one being deleted.
    
    I would suggest one of the alternatives:
    * create a copy
    * do two passes, instead of deleting mark the ranges and remove them at 
once during the second pass.
    
    Anyway, a short comment summarizing the algorithm would be great for 
posterity!



src/common/values.cpp (line 109)
<https://reviews.apache.org/r/38158/#comment154068>

    s/delete range/delete current ?
    
    How about rewriting this in active voice for clarity? "If `range` subsumes 
`current`, delete `current`"



src/common/values.cpp (line 138)
<https://reviews.apache.org/r/38158/#comment154070>

    An idea to re-use code here. This function boils down to inserting an 
element into a sorted range and then performing one step of coalescing from 
this element till the end of the sequence. We can keep the insetion "virual" 
and move the elements once during the second coalescing pass (see my comment 
above).



src/common/values.cpp (line 140)
<https://reviews.apache.org/r/38158/#comment154067>

    Let's pull it up to the function comment.



src/common/values.cpp (line 165)
<https://reviews.apache.org/r/38158/#comment154071>

    Want to transform this into a high level comment before the block?



src/common/values.cpp (lines 171 - 174)
<https://reviews.apache.org/r/38158/#comment154072>

    Nice diagram! Helps a lot.



src/common/values.cpp (lines 184 - 185)
<https://reviews.apache.org/r/38158/#comment154073>

    Since you're editing here, mind explaining what an "urange" is?



src/common/values.cpp (line 237)
<https://reviews.apache.org/r/38158/#comment154074>

    I though you don't like post-increments : )



src/common/values.cpp (lines 344 - 346)
<https://reviews.apache.org/r/38158/#comment154075>

    Let's let compiler do it's job: how about passing `left` by value and not 
creating a copy manually?



src/common/values.cpp (line 360)
<https://reviews.apache.org/r/38158/#comment154063>

    Does it make sense to optimize out this call? IIUC, `Values::Ranges` should 
be always in sorted and coalesced state ("normalized") unless modified manually 
via protobuf methods. We can require normalization for `operator+=()` to 
succeed. Similar to how you require sorting for coalescing. I would suggest 
let's define invariants (or pre-conditions and post-conditions) for some 
crucial methods, like operators, `sort()`, `coalesce()` and document them.
    
    If you prefer to leave it, then IIUC you should also `sort()` before 
`coalesce()`.



src/common/values.cpp (line 546)
<https://reviews.apache.org/r/38158/#comment154062>

    Just to make sure, you have added just `2` lines to this function (not easy 
on RB):
    ```
          sort(ranges);
          coalesce(ranges);
    ```
    Correct?



src/common/values.cpp (lines 630 - 637)
<https://reviews.apache.org/r/38158/#comment154058>

    I've got confused by two syntacticly same by semantically different 
`begin()`s and `end()`s. Let's leave a note for posterity hinting the 
difference.
    
    Also, `clang-format` suggests the following:
    ```
    void sort(Value::Ranges* ranges)
    {
      std::sort(ranges->mutable_range()->begin(),
                ranges->mutable_range()->end(),
                [](const Value::Range& a,
                   const Value::Range& b) { return a.begin() < b.begin(); });
    }
    ```



src/tests/values_tests.cpp (line 128)
<https://reviews.apache.org/r/38158/#comment154044>

    Not sure I follow the comment. Did you mean you cannot use `parse()` 
because it will `coalesce()`, while you want to test `sort()` in isolation?



src/tests/values_tests.cpp (line 131)
<https://reviews.apache.org/r/38158/#comment154045>

    Let's document what range is created here, like in the `RangesCoalesce` 
test.



src/tests/values_tests.cpp (line 158)
<https://reviews.apache.org/r/38158/#comment154047>

    How about an overlapping range, like `[1-4, 3-6]`? I see you check this in 
the next one, but since you have an isolated test here, why not slightly 
modifying the range to test one more condition?



src/tests/values_tests.cpp (lines 176 - 177)
<https://reviews.apache.org/r/38158/#comment154046>

    Don't you want to do a full check here?



src/tests/values_tests.cpp (lines 244 - 245)
<https://reviews.apache.org/r/38158/#comment154048>

    It looks like you never used `parsed2`, nor compare you `parsed` and 
`parsed1`. Do you want to use just `parsed` and `expected`?



src/tests/values_tests.cpp (lines 251 - 252)
<https://reviews.apache.org/r/38158/#comment154049>

    A typo?



src/tests/values_tests.cpp (line 255)
<https://reviews.apache.org/r/38158/#comment154050>

    Kill the newline, please.



src/tests/values_tests.cpp (line 271)
<https://reviews.apache.org/r/38158/#comment154051>

    one space between the arguments, please!



src/tests/values_tests.cpp (lines 273 - 274)
<https://reviews.apache.org/r/38158/#comment154052>

    Is it enough to check the size?



src/tests/values_tests.cpp (line 280)
<https://reviews.apache.org/r/38158/#comment154053>

    I'm not sure you should double 'p' in this case. Not a native speaker 
though.


- Alexander Rukletsov


On Sept. 7, 2015, 6:04 p.m., Joerg Schad wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2015, 6:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
>     https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -----
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Reply via email to