mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 22796: Add timeout to rescind unused offers
Date Wed, 09 Jul 2014 07:15:40 GMT

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


Looks great. Just a few minor nits, and a request for an offer_timeout=0 test, then we can
ship it!


src/master/flags.hpp
<https://reviews.apache.org/r/22796/#comment83343>

    Mention that offer_timeout=0 means never timeout.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83344>

    Extra blank line. Use 2 here, not 3.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83345>

    If you're not modifying the default masterFlags, you can just call StartMaster() and it
will CreateMasterFlags() for you.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83346>

    indent



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83347>

    Ditto. Just use StartSlave() if you're not modifying the default flags



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83348>

    StartMaster()



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83350>

    indent



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83349>

    StartSlave()



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83352>

    How about something a little more descriptive than 'message'?



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83351>

    Times(1) is implicit. You can remove this line.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83358>

    Weird test. According to your comment below, "We expect rescind to be called from unregister
slave". So this comment is inaccurate. And you're not even testing your offer_timeout here
then.
    I'd suggest instead, a test when offer_timeout=0 that the offer is never rescinded.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83353>

    StartMaster()



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83354>

    indent



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83355>

    StartSlave()



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83356>

    Times(1) is implicit, unnecessary



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83357>

    indent



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83360>

    BUG: Declare the FUTURE_PROTOBUF for SlaveRegisteredMessage before you call StartSlave,
else it might happen before you setup your future expectation.



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83359>

    This is the old-school verbose way of launching a task. Find an example of the LaunchTasks()
test action in one of these other tests, like SlaveTest.TerminatingSlaveDoesNotReregister



src/tests/master_tests.cpp
<https://reviews.apache.org/r/22796/#comment83361>

    Times(1) is implicit, unnecessary


- Adam B


On July 2, 2014, 5:42 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22796/
> -----------------------------------------------------------
> 
> (Updated July 2, 2014, 5:42 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-186
>     https://issues.apache.org/jira/browse/MESOS-186
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Based on Kapil's patch (https://reviews.apache.org/r/22066/), adding timeout for each
offer from master to remove the offer when it's no longer used.
> 
> 
> Diffs
> -----
> 
>   src/master/flags.hpp 32704ce 
>   src/master/master.hpp 5fef354 
>   src/master/master.cpp 251b699 
>   src/tests/master_tests.cpp 5a1cf7f 
> 
> Diff: https://reviews.apache.org/r/22796/diff/
> 
> 
> Testing
> -------
> 
> Added three more unit tests from Kapil's patch: Testing offer not rescinded after task
launched, offer not rescinded when framework/slave unregistered.
> The test exposed a race condition that can lead to a segfault if two remove offers are
called on the same offer.
> 
> make check.
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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