mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.
Date Mon, 07 Nov 2016 05:25:01 GMT

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




src/examples/persistent_volume_framework.cpp 
<https://reviews.apache.org/r/45962/#comment223905>

    Keep the TODO?



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

    s/shared/(possibly shared)/



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

    Keep the name?
    
    Shared persistent volume is a kind of persistent volume.



src/examples/persistent_volume_framework.cpp (lines 137 - 143)
<https://reviews.apache.org/r/45962/#comment223943>

    Here we can just create regular shards and shared shards based on input and one after
another. No interleaving necessary. See comments on the flags below.



src/examples/persistent_volume_framework.cpp (lines 186 - 187)
<https://reviews.apache.org/r/45962/#comment223944>

    Why is "shared-only" a possible value? A shard has only one persistent volume so it's
either "shared" or not. How about just a `bool isShared`. (As suggested below, we can put
it in the volume).



src/examples/persistent_volume_framework.cpp (lines 206 - 207)
<https://reviews.apache.org/r/45962/#comment223960>

    Any need for `string task_id`? If so it needs to be named `taskId` but I don't see the
need? Once it's assigned to the task we can get it out by `task.task_id()`.



src/examples/persistent_volume_framework.cpp (lines 213 - 220)
<https://reviews.apache.org/r/45962/#comment223949>

    No need to act differently based on the shard type.
    
    We can just have 
    
    - The first task (`launched == 0`) writes to the file: `echo hello > volume/persisted`
    - Later tasks read from the file: `cat volume/persisted`
    
    For regular persistent volumes the two tasks will be sequential, for shared ones they'll
be simulatenous. The fact that 1st task could finish before the 2nd is launched is not important:
the example framework mainly serves the purpose of demostrating the usage of the feature and
we don't want the tasks to block the CI for too long.
    
    As a potentially followup we can take a read/consumer command and a write/producer command
from flags (with default values). So CI would use the default values to complete quickly while
a human could try out different write and reads commands which could represent more interesting/edge
cases.



src/examples/persistent_volume_framework.cpp (lines 455 - 461)
<https://reviews.apache.org/r/45962/#comment223974>

    I think it's sufficient to have the following states. (We should use a minimun number
of state, we can always add more when more features are added to persistent volumes which
demand more state but starting with a large number of states makes it difficult to evolve
the test).
    
    ```
          STAGING = 0, // The shard is awaiting offers to launch more tasks.
          RUNNING,     // The shard is fully running (all its tasks are launched).
          TERMINATING, // The shard is terminating and needs to clean up its persistent volume.
          DONE         // The shard is terminated.
    ```
    
    This translates to:
    
    ```
          STAGING = 0, // In resourceOffers: launch tasks
          RUNNING,     // In resourceOffers: noop; In statusUpdate: check shard TERMINATING
condition.
          TERMINATING, // In resourceOffers: DESTROY
          DONE         // Test terminal condition.
    ```



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

    



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

    To represent a state, use a `-ing` word?
    
    See the overall comment on the enum.



src/examples/persistent_volume_framework.cpp (lines 460 - 461)
<https://reviews.apache.org/r/45962/#comment223963>

    Why both `WAIT_DONE` and `DONE`?
    
    Doesn't look like we need to treat shared pv and regular pv differently.
    
    See the overall comment on the enum.



src/examples/persistent_volume_framework.cpp (lines 465 - 475)
<https://reviews.apache.org/r/45962/#comment224960>

    I understand this is "just a test" but it would be cleaner the following way.
    
    ```
    struct Volume
    {
      explicit Volume(bool _isShared)
        : isShared(_isShared) {}
      
      Option<string> id;
      Option<string> slave;
      bool isShared;
    }
    ```
    
    Here the volume is really the "intention" for the persistent volume so `isShared` is known
when the shard is created and the optional values are filled when known.



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

    s/taskId/taskIds/?
    
    ```
    // The IDs of the tasks running on this shard?
    ```



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

    



src/examples/persistent_volume_framework.cpp (lines 533 - 546)
<https://reviews.apache.org/r/45962/#comment224959>

    To avoid any "magic" in determining the type/order/mode this test uses, why don't we just
use 
    
    ```
    --num_shards=3 // Number of regular (non-shared) shards the framework will run. 
    --num_shared_shards=3 // Number of shared shards the framework will run. Shared shards
are shards which use shared persistent volumes.
    ```
    
    We can use some default values (3 & 3 as shown above) and the user can customize however
they want.



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

    Why remove comments?



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

    Revert this.


- Jiang Yan Xu


On Oct. 21, 2016, 11:52 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2016, 11:52 a.m.)
> 
> 
> Review request for mesos 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
> 
>


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