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


Fix it, then Ship it!




Looks good! :)


src/examples/persistent_volume_framework.cpp (line 86)
<https://reviews.apache.org/r/45962/#comment228040>

    An empty line here before the return.



src/examples/persistent_volume_framework.cpp (line 122)
<https://reviews.apache.org/r/45962/#comment228173>

    s/In case/In the case/



src/examples/persistent_volume_framework.cpp (line 124)
<https://reviews.apache.org/r/45962/#comment228174>

    s/in case/in the case/



src/examples/persistent_volume_framework.cpp (line 150)
<https://reviews.apache.org/r/45962/#comment228186>

    As Gaston suggested, embeding sharedness in the name probably makes it more 
readable in the logs. Maybe `"shared-shard-"`?
    
    If we do that, no need to do `i + numShards`? Just do `stringify(i)`?



src/examples/persistent_volume_framework.cpp (lines 196 - 198)
<https://reviews.apache.org/r/45962/#comment228188>

    Use `Shard::INIT` as discussed.
    
    Then we can probably remove the comment. Consider the state machine already 
initialized (to INIT).



src/examples/persistent_volume_framework.cpp (lines 217 - 218)
<https://reviews.apache.org/r/45962/#comment228189>

    Add a TODO for the custom write command flag here to pair with the TODO 
below?



src/examples/persistent_volume_framework.cpp (line 254)
<https://reviews.apache.org/r/45962/#comment228191>

    Move this empty line to above line 253?



src/examples/persistent_volume_framework.cpp (lines 264 - 265)
<https://reviews.apache.org/r/45962/#comment228183>

    Can we briefly explain the reason: the writer task, although launched by 
the scheduler first, may have yet to write to the disk.



src/examples/persistent_volume_framework.cpp (line 267)
<https://reviews.apache.org/r/45962/#comment228225>

    Nit: "specifying a custom" reads better than "specifying a specific"?



src/examples/persistent_volume_framework.cpp (lines 269 - 272)
<https://reviews.apache.org/r/45962/#comment228185>

    Can we use 
    
    ```
    R"~(
    )~"
    ```
    
    string so we can write the script with with line breaks and indentation as 
we would usually do?


- Jiang Yan Xu


On Nov. 28, 2016, 4:25 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2016, 4:25 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
>     https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -----
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> -------
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Reply via email to