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 50000: Added test to simulate slow/unresponsive fetch.
Date Thu, 21 Jul 2016 17:40:25 GMT

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




src/tests/slave_tests.cpp (line 269)
<https://reviews.apache.org/r/50000/#comment208750>

    No `ROOT_` necessary.



src/tests/slave_tests.cpp (line 283)
<https://reviews.apache.org/r/50000/#comment208613>

    s/Mock native hadoop library/the mock hadoop binary that sleeps forever/
    
    so it's more clear?



src/tests/slave_tests.cpp (line 284)
<https://reviews.apache.org/r/50000/#comment208753>

    So on macOS `sleep infinity` wouldn't work so let's just do `sleep 1000`.



src/tests/slave_tests.cpp (line 291)
<https://reviews.apache.org/r/50000/#comment208614>

    We don't need this variable. Construct a nonexistent path directly below.



src/tests/slave_tests.cpp (line 294)
<https://reviews.apache.org/r/50000/#comment208754>

    It's already done in CreateSlaveFlags() so let's remove it from here.



src/tests/slave_tests.cpp (line 298)
<https://reviews.apache.org/r/50000/#comment208755>

    An empty line above.



src/tests/slave_tests.cpp (lines 305 - 307)
<https://reviews.apache.org/r/50000/#comment208756>

    Fix indentation.
    
    ```
    Try<Owned<cluster::Slave>> slave = StartSlave(
        detector.get(),
        containerizer.get(),
        flags);
    ```



src/tests/slave_tests.cpp (lines 311 - 312)
<https://reviews.apache.org/r/50000/#comment208757>

    No space between `driver` and `(`.
    
    Also we don't use this indentation style according to the style guide: http://mesos.apache.org/documentation/latest/c++-style-guide/
    
    Choose
    
    ```
    MesosSchedulerDriver driver(
        &sched,
        DEFAULT_FRAMEWORK_INFO,
        master.get()->pid,
        DEFAULT_CREDENTIAL);
    ```
    
    or 
    
    ```
    MesosSchedulerDriver driver(
        &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL);
    ```



src/tests/slave_tests.cpp (line 314)
<https://reviews.apache.org/r/50000/#comment208758>

    Always put expectations on the next line with two space indentation.
    
    ```
    EXPECT_CALL(sched, registered(&driver, _, _))
      .Times(1);
    ```



src/tests/slave_tests.cpp (lines 317 - 318)
<https://reviews.apache.org/r/50000/#comment208760>

    Formatting.
    
    ```
    EXPECT_CALL(sched, resourceOffers(&driver, _))
      .WillOnce(FutureArg<1>(&offers))
      .WillRepeatedly(Return()); // Ignore subsequent offers.
    ```



src/tests/slave_tests.cpp (lines 326 - 330)
<https://reviews.apache.org/r/50000/#comment208822>

    No space between method name and the open paren, here and elsewhere. 
    
    Plus, I think we can just use `createTask()` to generate the TaskInfo (like you did in
[here](https://github.com/apache/mesos/blob/a95ab8ba140f4554c0f9417dc10a7463f5df71d3/src/tests/gc_tests.cpp#L906))
but use the [CommandInfo version](https://github.com/apache/mesos/blob/a95ab8ba140f4554c0f9417dc10a7463f5df71d3/src/tests/mesos.hpp#L430-L436)?



src/tests/slave_tests.cpp (line 334)
<https://reviews.apache.org/r/50000/#comment208823>

    Just use a dummy value: "hdfs://dummyhost/dummypath"



src/tests/slave_tests.cpp (line 335)
<https://reviews.apache.org/r/50000/#comment208826>

    I guess "sleep 10" is as good as any other dummy value but let's add a comment:
    
    // Using a dummy command value as it's a required field.



src/tests/slave_tests.cpp (lines 344 - 345)
<https://reviews.apache.org/r/50000/#comment208827>

    This is too jagged, instead do:
    
    ```
    Future<Nothing> fetch = FUTURE_DISPATCH(
        _, &FetcherProcess::fetch);
    ```
    
    `fetch` should suffice as the variable name.



src/tests/slave_tests.cpp (line 355)
<https://reviews.apache.org/r/50000/#comment208831>

    This has a slight chance of causing flakyness because ExitedExecutorMessage is sent after
the status update. We can just wait for it as you would wait for the status update before
exiting the test.



src/tests/slave_tests.cpp (lines 358 - 360)
<https://reviews.apache.org/r/50000/#comment208828>

    Ditto (jaggedness).



src/tests/slave_tests.cpp (line 376)
<https://reviews.apache.org/r/50000/#comment208830>

    No need to log here.



src/tests/slave_tests.cpp (line 378)
<https://reviews.apache.org/r/50000/#comment208832>

    `status->source()`, i.e., no space.


- Jiang Yan Xu


On July 13, 2016, 4:33 p.m., Megha Sharma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50000/
> -----------------------------------------------------------
> 
> (Updated July 13, 2016, 4:33 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5763
>     https://issues.apache.org/jira/browse/MESOS-5763
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test to simulate the scenario of slow/unresponsive HDFS leading
> to executor register timeout and verify that slave gets notified of the
> failure.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_tests.cpp 60f9e1644efaeba893f4ff38b6d5a07087d1a355 
> 
> Diff: https://reviews.apache.org/r/50000/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>


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