Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-12 Thread Alexander Rukletsov 

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


Ship it!




Ship It!

- Alexander Rukletsov


On April 9, 2016, 2:45 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated April 9, 2016, 2:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 0bbb7ac9e80234920814e34286ea0da9b648ebe3 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-09 Thread Jian Qiu 


> On 二月 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 396
> > 
> >
> > Is there a need to explicitly invoke `Clock::resume()` here? If not, 
> > kill it.
> 
> Alexander Rukletsov wrote:
> Let's leave the test with resumed clock for consistency.
> 
> Anand Mazumdar wrote:
> Alex, what consistency are you referring to here? Does this file follow 
> the pattern of explicitly doing so?
> 
> Alexander Rukletsov wrote:
> We've discussed it with BenM here recently: 
> https://reviews.apache.org/r/44994/#comment186808
> 
> Anand Mazumdar wrote:
> Don't we explicitly resume the clock at the end of the test if an 
> assertion fails making the `resume` call redundant? 
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/gtest.hpp#L47
> 
> Looks like I am missing something here.

Change the test here to explicitly destroy container instead of waiting for 
timeout.


- Jian


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


On 四月 9, 2016, 2:45 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 四月 9, 2016, 2:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 0bbb7ac9e80234920814e34286ea0da9b648ebe3 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-09 Thread Jian Qiu 


> On 四月 8, 2016, 2:52 p.m., Alexander Rukletsov wrote:
> > src/tests/hook_tests.cpp, line 392
> > 
> >
> > we don't need `settle` here because AWAIT_READY does it for us. 
> > However, we should resume the clock since in the future we may have a check 
> > that the clock is unpaused when test finishes.

Since we explicitly destroy the container, there is no need to pause/resume 
clock here.


- Jian


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


On 四月 9, 2016, 2:45 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 四月 9, 2016, 2:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 0bbb7ac9e80234920814e34286ea0da9b648ebe3 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-09 Thread Jian Qiu 

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

(Updated 四月 9, 2016, 2:45 p.m.)


Review request for mesos, Alexander Rukletsov and Timothy Chen.


Changes
---

Address alex's comments.


Bugs: MESOS-4174
https://issues.apache.org/jira/browse/MESOS-4174


Repository: mesos


Description
---

Speed up HookTest.VerifySlaveLaunchExecutorHook.


Diffs (updated)
-

  src/tests/hook_tests.cpp 0bbb7ac9e80234920814e34286ea0da9b648ebe3 

Diff: https://reviews.apache.org/r/42241/diff/


Testing
---

Before
HookTest.VerifySlaveLaunchExecutorHook (5061 ms)

After
HookTest.VerifySlaveLaunchExecutorHook (132 ms)


Thanks,

Jian Qiu

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-08 Thread Alexander Rukletsov 

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




src/tests/hook_tests.cpp (line 391)


I would argue that the "right thing to do" here is to explicitly call 
`containerizer.destroy`. `TestContainerizer` does not track whether an 
underlying executor has exited or not, hence we should manually destroy the 
container *after* executor shuts down. What you propose to do, is to wait until 
the agent forcibly shuts down the executor, which is fine, but not necessary, 
and hence may be misleading. Does it make sense?



src/tests/hook_tests.cpp (line 392)


we don't need `settle` here because AWAIT_READY does it for us. However, we 
should resume the clock since in the future we may have a check that the clock 
is unpaused when test finishes.


- Alexander Rukletsov


On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-08 Thread Anand Mazumdar 


> On Feb. 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 396
> > 
> >
> > Is there a need to explicitly invoke `Clock::resume()` here? If not, 
> > kill it.
> 
> Alexander Rukletsov wrote:
> Let's leave the test with resumed clock for consistency.
> 
> Anand Mazumdar wrote:
> Alex, what consistency are you referring to here? Does this file follow 
> the pattern of explicitly doing so?
> 
> Alexander Rukletsov wrote:
> We've discussed it with BenM here recently: 
> https://reviews.apache.org/r/44994/#comment186808

Don't we explicitly resume the clock at the end of the test if an assertion 
fails making the `resume` call redundant? 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/gtest.hpp#L47

Looks like I am missing something here.


- Anand


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


On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-08 Thread Alexander Rukletsov 


> On Feb. 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 396
> > 
> >
> > Is there a need to explicitly invoke `Clock::resume()` here? If not, 
> > kill it.
> 
> Alexander Rukletsov wrote:
> Let's leave the test with resumed clock for consistency.
> 
> Anand Mazumdar wrote:
> Alex, what consistency are you referring to here? Does this file follow 
> the pattern of explicitly doing so?

We've discussed it with BenM here recently: 
https://reviews.apache.org/r/44994/#comment186808


- Alexander


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


On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-08 Thread Anand Mazumdar 


> On Feb. 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 396
> > 
> >
> > Is there a need to explicitly invoke `Clock::resume()` here? If not, 
> > kill it.
> 
> Alexander Rukletsov wrote:
> Let's leave the test with resumed clock for consistency.

Alex, what consistency are you referring to here? Does this file follow the 
pattern of explicitly doing so?


- Anand


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


On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-04-08 Thread Alexander Rukletsov 


> On Feb. 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 396
> > 
> >
> > Is there a need to explicitly invoke `Clock::resume()` here? If not, 
> > kill it.

Let's leave the test with resumed clock for consistency.


- Alexander


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


On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-02-19 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [42241]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-02-18 Thread Anand Mazumdar 

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


Ship it!




Ship It!

- Anand Mazumdar


On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-02-18 Thread Jian Qiu 


> On 二月 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 390
> > 
> >
> > hmm ... Can't we just do:
> > 
> > ```
> > Clock::pause();
> > Clock::advance(...);
> > Clock::settle();
> > ```
> 
> Jian Qiu wrote:
> Thanks for reviewing! This settle() is necessary to trigger the 
> shutdownExecutor timer.
> 
> Anand Mazumdar wrote:
> Ahh, I see. It's not immediatly obvious to to why we need the first 
> `Clock::settle`. As a suggestion, can we add a comment like this:
> 
> ```
> Clock::settle(); // Wait for `Slave::shutdownExecutor` to complete.
> ```

Done :)


- Jian


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


On 二月 19, 2016, 4:36 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 二月 19, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-02-18 Thread Anand Mazumdar 


> On Feb. 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 390
> > 
> >
> > hmm ... Can't we just do:
> > 
> > ```
> > Clock::pause();
> > Clock::advance(...);
> > Clock::settle();
> > ```
> 
> Jian Qiu wrote:
> Thanks for reviewing! This settle() is necessary to trigger the 
> shutdownExecutor timer.

Ahh, I see. It's not immediatly obvious to to why we need the first 
`Clock::settle`. As a suggestion, can we add a comment like this:

```
Clock::settle(); // Wait for `Slave::shutdownExecutor` to complete.
```


- Anand


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


On Feb. 19, 2016, 3:29 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Feb. 19, 2016, 3:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-02-18 Thread Jian Qiu 


> On 二月 19, 2016, 2:50 a.m., Anand Mazumdar wrote:
> > src/tests/hook_tests.cpp, line 390
> > 
> >
> > hmm ... Can't we just do:
> > 
> > ```
> > Clock::pause();
> > Clock::advance(...);
> > Clock::settle();
> > ```

Thanks for reviewing! This settle() is necessary to trigger the 
shutdownExecutor timer.


- Jian


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


On 二月 19, 2016, 3:29 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 二月 19, 2016, 3:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-02-18 Thread Jian Qiu 

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

(Updated 二月 19, 2016, 3:29 a.m.)


Review request for mesos, Alexander Rukletsov and Timothy Chen.


Changes
---

address @anandmazumdar comments


Bugs: MESOS-4174
https://issues.apache.org/jira/browse/MESOS-4174


Repository: mesos


Description
---

Speed up HookTest.VerifySlaveLaunchExecutorHook.


Diffs (updated)
-

  src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 

Diff: https://reviews.apache.org/r/42241/diff/


Testing
---

Before
HookTest.VerifySlaveLaunchExecutorHook (5061 ms)

After
HookTest.VerifySlaveLaunchExecutorHook (132 ms)


Thanks,

Jian Qiu

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-02-18 Thread Anand Mazumdar 

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



LGTM, some minor comments around avoiding redundant use of `Clock` functions 
that can be avoided.


src/tests/hook_tests.cpp (line 390)


hmm ... Can't we just do:

```
Clock::pause();
Clock::advance(...);
Clock::settle();
```



src/tests/hook_tests.cpp (line 392)


Missing period at the end.



src/tests/hook_tests.cpp (line 396)


Is there a need to explicitly invoke `Clock::resume()` here? If not, kill 
it.


- Anand Mazumdar


On Jan. 18, 2016, 4:03 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Jan. 18, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-18 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [42241]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 18, 2016, 4:03 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Jan. 18, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-18 Thread Alexander Rukletsov 

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



src/tests/hook_tests.cpp (lines 396 - 399)


Why do you need a loop here? Isn't it enought to advance once and then 
settle?


- Alexander Rukletsov


On Jan. 18, 2016, 2:28 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Jan. 18, 2016, 2:28 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-18 Thread Jian Qiu 

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

(Updated 一月 18, 2016, 4:03 p.m.)


Review request for mesos, Alexander Rukletsov and Timothy Chen.


Bugs: MESOS-4174
https://issues.apache.org/jira/browse/MESOS-4174


Repository: mesos


Description
---

Speed up HookTest.VerifySlaveLaunchExecutorHook.


Diffs (updated)
-

  src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 

Diff: https://reviews.apache.org/r/42241/diff/


Testing
---

Before
HookTest.VerifySlaveLaunchExecutorHook (5061 ms)

After
HookTest.VerifySlaveLaunchExecutorHook (132 ms)


Thanks,

Jian Qiu

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-18 Thread Jian Qiu 


> On 一月 18, 2016, 11:49 a.m., Alexander Rukletsov wrote:
> > src/tests/hook_tests.cpp, lines 396-399
> > 
> >
> > Why do you need a loop here? Isn't it enought to advance once and then 
> > settle?

Right...there is no need to loop. Thanks for reminding!


- Jian


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


On 一月 18, 2016, 4:03 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 一月 18, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-18 Thread Jian Qiu 


> On 一月 15, 2016, 8:53 p.m., Timothy Chen wrote:
> > src/tests/hook_tests.cpp, line 389
> > 
> >
> > If hookFuture never becomes ready that this blocks forever right?
> > AWAIT_READY has a timeout built in, and we will want to also make sure 
> > we don't hang forever.
> > Also do we have a logically expected time the hook future should be 
> > ready? Can we just advance once and then AWAIT?
> 
> Jian Qiu wrote:
> Thanks, @tnachen. the while loop here is to wait for the executor being 
> terminated. Instead of checking hookFuture in while loop, how about exepect a 
> call to executorTerminated, and check whether it is called in while loop?

After rechecking the test case, you are right that we should not use a loop 
here.


- Jian


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


On 一月 18, 2016, 4:03 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 一月 18, 2016, 4:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-17 Thread Jian Qiu 


> On 一月 15, 2016, 8:53 p.m., Timothy Chen wrote:
> > src/tests/hook_tests.cpp, line 389
> > 
> >
> > If hookFuture never becomes ready that this blocks forever right?
> > AWAIT_READY has a timeout built in, and we will want to also make sure 
> > we don't hang forever.
> > Also do we have a logically expected time the hook future should be 
> > ready? Can we just advance once and then AWAIT?

Thanks, @tnachen. the while loop here is to wait for the executor being 
terminated. Instead of checking hookFuture in while loop, how about exepect a 
call to executorTerminated, and check whether it is called in while loop?


- Jian


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


On 一月 18, 2016, 2:28 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 一月 18, 2016, 2:28 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-17 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [42241]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 18, 2016, 2:28 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Jan. 18, 2016, 2:28 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Speed up HookTest.VerifySlaveLaunchExecutorHook.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-17 Thread Jian Qiu 

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

(Updated 一月 18, 2016, 2:28 a.m.)


Review request for mesos, Alexander Rukletsov and Timothy Chen.


Bugs: MESOS-4174
https://issues.apache.org/jira/browse/MESOS-4174


Repository: mesos


Description (updated)
---

Speed up HookTest.VerifySlaveLaunchExecutorHook.


Diffs (updated)
-

  src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 

Diff: https://reviews.apache.org/r/42241/diff/


Testing
---

Before
HookTest.VerifySlaveLaunchExecutorHook (5061 ms)

After
HookTest.VerifySlaveLaunchExecutorHook (132 ms)


Thanks,

Jian Qiu

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-15 Thread Jian Qiu 

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

(Updated Jan. 15, 2016, 6:20 p.m.)


Review request for mesos, Alexander Rukletsov and Timothy Chen.


Changes
---

Looks like this review depends on an unpublished review. Fixed it to unblock 
the Review Bot. -- @vinodkone


Bugs: MESOS-4174
https://issues.apache.org/jira/browse/MESOS-4174


Repository: mesos


Description
---

Add Clock::advance to fasten the speed of executor to be shutdown.


Diffs
-

  src/tests/hook_tests.cpp 715b0b07e1befdca5b09172a439edccd7546d77f 

Diff: https://reviews.apache.org/r/42241/diff/


Testing
---

Before
HookTest.VerifySlaveLaunchExecutorHook (5061 ms)

After
HookTest.VerifySlaveLaunchExecutorHook (132 ms)


Thanks,

Jian Qiu

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-15 Thread Timothy Chen 

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



src/tests/hook_tests.cpp (line 389)


If hookFuture never becomes ready that this blocks forever right?
AWAIT_READY has a timeout built in, and we will want to also make sure we 
don't hang forever.
Also do we have a logically expected time the hook future should be ready? 
Can we just advance once and then AWAIT?


- Timothy Chen


On Jan. 15, 2016, 6:20 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Jan. 15, 2016, 6:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Clock::advance to fasten the speed of executor to be shutdown.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 715b0b07e1befdca5b09172a439edccd7546d77f 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-15 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [42241]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 15, 2016, 6:20 p.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Jan. 15, 2016, 6:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Clock::advance to fasten the speed of executor to be shutdown.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 715b0b07e1befdca5b09172a439edccd7546d77f 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-14 Thread Guangya Liu 

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

Ship it!


Ship It!

- Guangya Liu


On 一月 15, 2016, 5:10 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated 一月 15, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Clock::advance to fasten the speed of executor to be shutdown.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 715b0b07e1befdca5b09172a439edccd7546d77f 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-14 Thread haosdent huang 

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

Ship it!


Ship It!

- haosdent huang


On Jan. 15, 2016, 5:10 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42241/
> ---
> 
> (Updated Jan. 15, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Timothy Chen.
> 
> 
> Bugs: MESOS-4174
> https://issues.apache.org/jira/browse/MESOS-4174
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Clock::advance to fasten the speed of executor to be shutdown.
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 715b0b07e1befdca5b09172a439edccd7546d77f 
> 
> Diff: https://reviews.apache.org/r/42241/diff/
> 
> 
> Testing
> ---
> 
> Before
> HookTest.VerifySlaveLaunchExecutorHook (5061 ms)
> 
> After
> HookTest.VerifySlaveLaunchExecutorHook (132 ms)
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.

2016-01-13 Thread Jian Qiu 

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

Review request for mesos and Alexander Rukletsov.


Bugs: MESOS-4174
https://issues.apache.org/jira/browse/MESOS-4174


Repository: mesos


Description
---

Add Clock::advance to fasten the speed of executor to be shutdown.


Diffs
-

  src/tests/hook_tests.cpp 715b0b07e1befdca5b09172a439edccd7546d77f 

Diff: https://reviews.apache.org/r/42241/diff/


Testing
---

Before
HookTest.VerifySlaveLaunchExecutorHook (5061 ms)

After
HookTest.VerifySlaveLaunchExecutorHook (132 ms)


Thanks,

Jian Qiu