You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2013/11/22 02:48:43 UTC

Review Request 15773: Fixed master to ignore messages from unregistered framework.

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

Review request for mesos, Benjamin Hindman and Ben Mahler.


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


Repository: mesos-git


Description
-------

See bug for details.


Diffs
-----

  src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
  src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
  src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 

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


Testing
-------

make check (Linux and OSX)

sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Vinod Kone


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15773/#review29403
-----------------------------------------------------------

Ship it!


Ship It! But looks like your diff needs a rebase?

- Ben Mahler


On Nov. 25, 2013, 10:38 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15773/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2013, 10:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-834
>     https://issues.apache.org/jira/browse/MESOS-834
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See bug for details.
> 
> 
> Diffs
> -----
> 
>   src/cli/mesos-cat f898e82b9a3e1ca5930838a6f32d83d09eea0e7a 
>   src/cli/mesos-tail e2aeaacc67bc30a363701db367e8561dc0b3efac 
>   src/log/replica.cpp 82c21579af423e6d83d8c03402a9a5d116e93b97 
>   src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
>   src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
>   src/messages/log.proto e6460aba77ea349a3430bbc8119925b0107a540e 
>   src/slave/http.cpp fbc917b1cfbe1a5cc7587d3f4f37e24271660536 
>   src/slave/main.cpp 7a184943c0b70e23a612f2c9c3c4f71e8c7dbb54 
>   src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 
> 
> Diff: https://reviews.apache.org/r/15773/diff/
> 
> 
> Testing
> -------
> 
> make check (Linux and OSX)
> 
> sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15773/
-----------------------------------------------------------

(Updated Nov. 25, 2013, 10:38 p.m.)


Review request for mesos, Benjamin Hindman and Ben Mahler.


Changes
-------

ok. i give up :) commented the steps above the test.


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


Repository: mesos-git


Description
-------

See bug for details.


Diffs (updated)
-----

  src/cli/mesos-cat f898e82b9a3e1ca5930838a6f32d83d09eea0e7a 
  src/cli/mesos-tail e2aeaacc67bc30a363701db367e8561dc0b3efac 
  src/log/replica.cpp 82c21579af423e6d83d8c03402a9a5d116e93b97 
  src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
  src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
  src/messages/log.proto e6460aba77ea349a3430bbc8119925b0107a540e 
  src/slave/http.cpp fbc917b1cfbe1a5cc7587d3f4f37e24271660536 
  src/slave/main.cpp 7a184943c0b70e23a612f2c9c3c4f71e8c7dbb54 
  src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 

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


Testing
-------

make check (Linux and OSX)

sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Vinod Kone


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15773/
-----------------------------------------------------------

(Updated Nov. 25, 2013, 7:46 p.m.)


Review request for mesos, Benjamin Hindman and Ben Mahler.


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


Repository: mesos-git


Description
-------

See bug for details.


Diffs
-----

  src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
  src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
  src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 

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


Testing
-------

make check (Linux and OSX)

sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Vinod Kone


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15773/
-----------------------------------------------------------

(Updated Nov. 25, 2013, 7:46 p.m.)


Review request for mesos, Benjamin Hindman and Ben Mahler.


Changes
-------

benm's.


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


Repository: mesos-git


Description
-------

See bug for details.


Diffs (updated)
-----

  src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
  src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
  src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 

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


Testing
-------

make check (Linux and OSX)

sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Vinod Kone


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Ian Downes <id...@twitter.com>.
On Nov 25, 2013, at 12:03 PM, "Ben Mahler" <be...@gmail.com> wrote
>> 
>> Vinod Kone wrote:
>>    I'm not convinced about repeating the specific details of "how" in the comment when it is already in the comments. I think our abstracts are pretty easy to understand what's happening (StartMaster(), StartSlave(), driver.start() etc).
>> 
>>    That said I added couple more comments in the test to make you happy :)
> 
> Appreciate it! Although I still don't see a summary about what this test is doing ;)
> 
> Think of this as a summary rather than repetition, sometimes we have to think pretty hard about how to test things that it's good to summarize this for others trying to understand the test. In complicated tests, having both what is being tested, along with how it's being tested at the top of the test seems useful, no?
> 

+1 on summarizing a test's purpose

As a newbie reading tests it'll make things much, much easier. Recreating a test's purpose from code plus test name is backwards to me. It'll also be clearer if we document which expectations are truly part of the test. 

Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Ben Mahler <be...@gmail.com>.

> On Nov. 25, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, line 1384
> > <https://reviews.apache.org/r/15773/diff/1/?file=389617#file389617line1384>
> >
> >     Ok, I like that this summary mentions what it is testing, but it does not mention how it is testing it, so I must read through the test and observe that you're starting a master, slave, framework, and then a second framework, dropping a message, using a kill task and a finished status to ensure the kill task is ignored.
> >     
> >     I think this is difficult to extract from reading the tests for those who are less well versed in our integration testing abstractions, so the numbered steps are nice for them :)
> >     
> >     I tried to follow this format here:
> >     https://reviews.apache.org/r/15778/diff/#1.2
> 
> Vinod Kone wrote:
>     I'm not convinced about repeating the specific details of "how" in the comment when it is already in the comments. I think our abstracts are pretty easy to understand what's happening (StartMaster(), StartSlave(), driver.start() etc).
>     
>     That said I added couple more comments in the test to make you happy :)

Appreciate it! Although I still don't see a summary about what this test is doing ;)

Think of this as a summary rather than repetition, sometimes we have to think pretty hard about how to test things that it's good to summarize this for others trying to understand the test. In complicated tests, having both what is being tested, along with how it's being tested at the top of the test seems useful, no?


- Ben


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


On Nov. 25, 2013, 7:46 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15773/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2013, 7:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-834
>     https://issues.apache.org/jira/browse/MESOS-834
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See bug for details.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
>   src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
>   src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 
> 
> Diff: https://reviews.apache.org/r/15773/diff/
> 
> 
> Testing
> -------
> 
> make check (Linux and OSX)
> 
> sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Ben Mahler <be...@gmail.com>.

> On Nov. 25, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1045
> > <https://reviews.apache.org/r/15773/diff/2/?file=390839#file390839line1045>
> >
> >     Looks like the LOG(INFO) was dropped in this diff?
> 
> Vinod Kone wrote:
>     Yea dropped it because it gets printed in deactivate(Framework*). I forgot to remove the LOG(INFO) when I refactored deactivate().

But since this is a pretty infrequent occasion, would it not be useful to know why we're deactivating a framework? I used this old log line today to distinguish a driver triggered deactivation.


- Ben


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


On Nov. 25, 2013, 7:46 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15773/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2013, 7:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-834
>     https://issues.apache.org/jira/browse/MESOS-834
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See bug for details.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
>   src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
>   src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 
> 
> Diff: https://reviews.apache.org/r/15773/diff/
> 
> 
> Testing
> -------
> 
> make check (Linux and OSX)
> 
> sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Vinod Kone <vi...@gmail.com>.

> On Nov. 25, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1045
> > <https://reviews.apache.org/r/15773/diff/2/?file=390839#file390839line1045>
> >
> >     Looks like the LOG(INFO) was dropped in this diff?

Yea dropped it because it gets printed in deactivate(Framework*). I forgot to remove the LOG(INFO) when I refactored deactivate().


> On Nov. 25, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1193
> > <https://reviews.apache.org/r/15773/diff/2/?file=390839#file390839line1193>
> >
> >     missing return?

good catch.


> On Nov. 25, 2013, 7:07 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, line 1384
> > <https://reviews.apache.org/r/15773/diff/1/?file=389617#file389617line1384>
> >
> >     Ok, I like that this summary mentions what it is testing, but it does not mention how it is testing it, so I must read through the test and observe that you're starting a master, slave, framework, and then a second framework, dropping a message, using a kill task and a finished status to ensure the kill task is ignored.
> >     
> >     I think this is difficult to extract from reading the tests for those who are less well versed in our integration testing abstractions, so the numbered steps are nice for them :)
> >     
> >     I tried to follow this format here:
> >     https://reviews.apache.org/r/15778/diff/#1.2

I'm not convinced about repeating the specific details of "how" in the comment when it is already in the comments. I think our abstracts are pretty easy to understand what's happening (StartMaster(), StartSlave(), driver.start() etc).

That said I added couple more comments in the test to make you happy :)


- Vinod


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


On Nov. 25, 2013, 6:56 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15773/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2013, 6:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-834
>     https://issues.apache.org/jira/browse/MESOS-834
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See bug for details.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
>   src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
>   src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 
> 
> Diff: https://reviews.apache.org/r/15773/diff/
> 
> 
> Testing
> -------
> 
> make check (Linux and OSX)
> 
> sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15773/#review29380
-----------------------------------------------------------


Looks like a return was missed in reviveOffers


src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/15773/#comment56580>

    Ok, I like that this summary mentions what it is testing, but it does not mention how it is testing it, so I must read through the test and observe that you're starting a master, slave, framework, and then a second framework, dropping a message, using a kill task and a finished status to ensure the kill task is ignored.
    
    I think this is difficult to extract from reading the tests for those who are less well versed in our integration testing abstractions, so the numbered steps are nice for them :)
    
    I tried to follow this format here:
    https://reviews.apache.org/r/15778/diff/#1.2



src/master/master.cpp
<https://reviews.apache.org/r/15773/#comment56581>

    Looks like the LOG(INFO) was dropped in this diff?



src/master/master.cpp
<https://reviews.apache.org/r/15773/#comment56582>

    missing return?


- Ben Mahler


On Nov. 25, 2013, 6:56 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15773/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2013, 6:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-834
>     https://issues.apache.org/jira/browse/MESOS-834
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See bug for details.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
>   src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
>   src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 
> 
> Diff: https://reviews.apache.org/r/15773/diff/
> 
> 
> Testing
> -------
> 
> make check (Linux and OSX)
> 
> sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15773/
-----------------------------------------------------------

(Updated Nov. 25, 2013, 6:56 p.m.)


Review request for mesos, Benjamin Hindman and Ben Mahler.


Changes
-------

benm's.


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


Repository: mesos-git


Description
-------

See bug for details.


Diffs (updated)
-----

  src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
  src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
  src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 

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


Testing
-------

make check (Linux and OSX)

sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure


Thanks,

Vinod Kone


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Vinod Kone <vi...@gmail.com>.

> On Nov. 23, 2013, 2:53 a.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, line 1384
> > <https://reviews.apache.org/r/15773/diff/1/?file=389617#file389617line1384>
> >
> >     Can you add the steps that this test takes in numerical order? Just so I know what this test is doing to make it easier to read :)

I typically write the gist of the test on the top. The specific steps are written as comments inside the test. That way there is no repetition. Let me know if the comments outside/inside are not clear.


> On Nov. 23, 2013, 2:53 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 1077
> > <https://reviews.apache.org/r/15773/diff/1/?file=389616#file389616line1077>
> >
> >     Maybe it's time to update resourceRequest, launchTasks, reviveOffers to log on NULL? Placing the rejection cases first might be more consistent:
> >     
> >     Framework* framework = getFramework(frameworkId);
> >     
> >     if (framework == NULL) {
> >       LOG(WARNING) << ...;
> >       return;
> >     }
> >     
> >     if (framework->pid != from) {
> >       LOG(WARNING) << ...;
> >       return;
> >     }
> >     
> >     LOG(INFO) << "Requesting resources for framework " << frameworkId;
> >     allocator->resourcesRequested(frameworkId, requests);

done


> On Nov. 23, 2013, 2:53 a.m., Ben Mahler wrote:
> > src/master/master.cpp, line 774
> > <https://reviews.apache.org/r/15773/diff/1/?file=389616#file389616line774>
> >
> >     Quoting from might be nice since UPID() prints nothing. Ditto elsewhere in the added messages.

I don't think 'from' or 'framework->pid' would ever be nothing, but ok, I'll quote them.


- Vinod


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


On Nov. 22, 2013, 1:48 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15773/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2013, 1:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-834
>     https://issues.apache.org/jira/browse/MESOS-834
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See bug for details.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
>   src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
>   src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 
> 
> Diff: https://reviews.apache.org/r/15773/diff/
> 
> 
> Testing
> -------
> 
> make check (Linux and OSX)
> 
> sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


Re: Review Request 15773: Fixed master to ignore messages from unregistered framework.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15773/#review29329
-----------------------------------------------------------



src/master/master.cpp
<https://reviews.apache.org/r/15773/#comment56513>

    Quoting from might be nice since UPID() prints nothing. Ditto elsewhere in the added messages.



src/master/master.cpp
<https://reviews.apache.org/r/15773/#comment56514>

    Maybe it's time to update resourceRequest, launchTasks, reviveOffers to log on NULL? Placing the rejection cases first might be more consistent:
    
    Framework* framework = getFramework(frameworkId);
    
    if (framework == NULL) {
      LOG(WARNING) << ...;
      return;
    }
    
    if (framework->pid != from) {
      LOG(WARNING) << ...;
      return;
    }
    
    LOG(INFO) << "Requesting resources for framework " << frameworkId;
    allocator->resourcesRequested(frameworkId, requests);



src/master/master.cpp
<https://reviews.apache.org/r/15773/#comment56515>

    I like how this one rejects with return first :)



src/tests/fault_tolerance_tests.cpp
<https://reviews.apache.org/r/15773/#comment56516>

    Can you add the steps that this test takes in numerical order? Just so I know what this test is doing to make it easier to read :)


- Ben Mahler


On Nov. 22, 2013, 1:48 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15773/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2013, 1:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Bugs: MESOS-834
>     https://issues.apache.org/jira/browse/MESOS-834
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See bug for details.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp c86c1f1e7eb23c168bcd2e8ab0a0701c421fc59d 
>   src/master/master.cpp a08d01208ff7bbb878b2d50d8406efee4de86171 
>   src/tests/fault_tolerance_tests.cpp 6cb5829119ed3e855740fb9331e4fb19c16d3629 
> 
> Diff: https://reviews.apache.org/r/15773/diff/
> 
> 
> Testing
> -------
> 
> make check (Linux and OSX)
> 
> sudo ./bin/mesos-tests.sh --gtest_filter="*IgnoreKillTaskFromUnregisteredFramework*"  --gtest_repeat=1000 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>