mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Re: Review Request 59545: Added a test to verify the agent flag 'executor_reregistration_timeout'.
Date Thu, 01 Jun 2017 19:37:16 GMT

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



Sorry, I couldn't resist a few pedantic coding style suggestions... I also realize that I'm
complaining about things that occur in other tests as well :)


src/tests/slave_tests.cpp
Lines 7255 (patched)
<https://reviews.apache.org/r/59545/#comment250049>

    Do we actually expect subsequent offers in the test, or is this just coding defensively?
(If the latter, I'm not sure it is good practice, since it might hide bugs or unexpected behavioral
changes.)



src/tests/slave_tests.cpp
Lines 7259 (patched)
<https://reviews.apache.org/r/59545/#comment250046>

    Do we actually need to wait for a batch allocation here?
    
    IMO it would be clearer to first prompt the agent to register (before connecting the framework);
then when the framework registers, it should immediately receive an offer.



src/tests/slave_tests.cpp
Lines 7263 (patched)
<https://reviews.apache.org/r/59545/#comment250051>

    Style-wise, I'd prefer `ASSERT_FALSE(offers.empty())`.



src/tests/slave_tests.cpp
Lines 7265 (patched)
<https://reviews.apache.org/r/59545/#comment250050>

    Style-wise, I'd prefer `offers->front()`.



src/tests/slave_tests.cpp
Lines 7322 (patched)
<https://reviews.apache.org/r/59545/#comment250053>

    Won't be marked `TASK_UNREACHABLE` because the framework is not PA; arguably confusing
to suggest that in the comment.



src/tests/slave_tests.cpp
Lines 7335 (patched)
<https://reviews.apache.org/r/59545/#comment250056>

    As above, if we don't expect additional status updates, not sure it is good practice to
add the `WillRepeatedly` expectation.



src/tests/slave_tests.cpp
Lines 7340 (patched)
<https://reviews.apache.org/r/59545/#comment250048>

    I'd argue for `EXPECT` here, not `ASSERT`.


- Neil Conway


On May 30, 2017, 11:29 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59545/
> -----------------------------------------------------------
> 
> (Updated May 30, 2017, 11:29 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-7540
>     https://issues.apache.org/jira/browse/MESOS-7540
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a test to verify the correct behavior of
> the agent flag 'executor_reregistration_timeout.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_tests.cpp 927b9c3fbc3373b49c799da6e3f770b73964f9e5 
> 
> 
> Diff: https://reviews.apache.org/r/59545/diff/5/
> 
> 
> Testing
> -------
> 
> `bin/mesos-tests.sh --gtest_filter="*ExecutorReregistrationTimeoutFlag*" --gtest_repeat=-1
--gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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