Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-21 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [58304, 58305, 58621, 58622, 58303]

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

- Mesos Reviewbot


On April 21, 2017, 5:04 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58303/
> ---
> 
> (Updated April 21, 2017, 5:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In all cases where we call Master::removeTask, we have the correct slave
> pointer in hand. We can just pass it down, avoiding the need to look it
> up again with the SlaveID.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
> 
> 
> Diff: https://reviews.apache.org/r/58303/diff/9/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-21 Thread James Peach 

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

(Updated April 21, 2017, 5:04 p.m.)


Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 


Diff: https://reviews.apache.org/r/58303/diff/9/

Changes: https://reviews.apache.org/r/58303/diff/8-9/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-21 Thread James Peach 

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

(Updated April 21, 2017, 4:57 p.m.)


Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 


Diff: https://reviews.apache.org/r/58303/diff/8/

Changes: https://reviews.apache.org/r/58303/diff/7-8/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-19 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [58304, 58305, 58303]

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

- Mesos Reviewbot


On April 19, 2017, 10:49 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58303/
> ---
> 
> (Updated April 19, 2017, 10:49 p.m.)
> 
> 
> Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In all cases where we call Master::removeTask, we have the correct slave
> pointer in hand. We can just pass it down, avoiding the need to look it
> up again with the SlaveID.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp 52de2f91bdacf46f913c27382ad50b4f278ad297 
> 
> 
> Diff: https://reviews.apache.org/r/58303/diff/7/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-19 Thread James Peach 

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

(Updated April 19, 2017, 10:49 p.m.)


Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp 52de2f91bdacf46f913c27382ad50b4f278ad297 


Diff: https://reviews.apache.org/r/58303/diff/7/

Changes: https://reviews.apache.org/r/58303/diff/6-7/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-19 Thread James Peach 

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

(Updated April 19, 2017, 10:04 p.m.)


Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp 52de2f91bdacf46f913c27382ad50b4f278ad297 


Diff: https://reviews.apache.org/r/58303/diff/6/

Changes: https://reviews.apache.org/r/58303/diff/5-6/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-18 Thread James Peach 

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

(Updated April 18, 2017, 10:14 p.m.)


Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.


Changes
---

Rebased and addressed review feedback.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp 0f4c64c6b102ef201779a331c96b5d78a98281ad 


Diff: https://reviews.apache.org/r/58303/diff/5/

Changes: https://reviews.apache.org/r/58303/diff/4-5/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-17 Thread Neil Conway 

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




src/master/master.hpp
Line 760 (original), 760 (patched)


Can you add a comment that `slave` might be `nullptr`?



src/master/master.cpp
Line 7886 (original), 7886 (patched)


Can you move this down below the `CHECK(!slaves.registered.contains(...))` 
call? i.e., seems to make more sense to me to assert that there's no slave we 
need to update before doing `updateTask()`.


- Neil Conway


On April 17, 2017, 4:14 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58303/
> ---
> 
> (Updated April 17, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In all cases where we call Master::removeTask, we have the correct slave
> pointer in hand. We can just pass it down, avoiding the need to look it
> up again with the SlaveID.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp 0f4c64c6b102ef201779a331c96b5d78a98281ad 
> 
> 
> Diff: https://reviews.apache.org/r/58303/diff/4/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-17 Thread James Peach 

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

(Updated April 17, 2017, 4:14 p.m.)


Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.


Changes
---

Rebase.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp 0f4c64c6b102ef201779a331c96b5d78a98281ad 


Diff: https://reviews.apache.org/r/58303/diff/4/

Changes: https://reviews.apache.org/r/58303/diff/3-4/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-11 Thread James Peach 

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

(Updated April 11, 2017, 10:31 p.m.)


Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.


Changes
---

Rebased and addressed review feedback.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 


Diff: https://reviews.apache.org/r/58303/diff/3/

Changes: https://reviews.apache.org/r/58303/diff/2-3/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-11 Thread James Peach 


> On April 11, 2017, 7:05 p.m., Neil Conway wrote:
> > I wonder if we should make a similar change to `Master::updateTask()`? I 
> > don't feel super strongly one way or another, but you could argue that it 
> > would improve symmetry.

Done.


- James


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


On April 11, 2017, 9:40 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58303/
> ---
> 
> (Updated April 11, 2017, 9:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Mesos Reviewbot, and Neil Conway.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In all cases where we call Master::removeTask, we have the correct slave
> pointer in hand. We can just pass it down, avoiding the need to look it
> up again with the SlaveID.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 
> 
> 
> Diff: https://reviews.apache.org/r/58303/diff/2/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-11 Thread Neil Conway 

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



I wonder if we should make a similar change to `Master::updateTask()`? I don't 
feel super strongly one way or another, but you could argue that it would 
improve symmetry.

- Neil Conway


On April 11, 2017, 4:27 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58303/
> ---
> 
> (Updated April 11, 2017, 4:27 p.m.)
> 
> 
> Review request for mesos and Mesos Reviewbot.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In all cases where we call Master::removeTask, we have the correct slave
> pointer in hand. We can just pass it down, avoiding the need to look it
> up again with the SlaveID.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 
> 
> 
> Diff: https://reviews.apache.org/r/58303/diff/2/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-11 Thread James Peach 

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

(Updated April 11, 2017, 4:27 p.m.)


Review request for mesos and Mesos Reviewbot.


Changes
---

Rebased and addresses review feedback.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 


Diff: https://reviews.apache.org/r/58303/diff/2/

Changes: https://reviews.apache.org/r/58303/diff/1-2/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-11 Thread Adam B 

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


Fix it, then Ship it!




LGTM, minor nit.


src/master/master.cpp
Lines 8435-8439 (original), 8435-8436 (patched)


Nit: I'd probably swap these CHECKs to match the parameter order (or swap 
the parameters).


- Adam B


On April 10, 2017, 8:48 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58303/
> ---
> 
> (Updated April 10, 2017, 8:48 a.m.)
> 
> 
> Review request for mesos and Mesos Reviewbot.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In all cases where we call Master::removeTask, we have the correct slave
> pointer in hand. We can just pass it down, avoiding the need to look it
> up again with the SlaveID.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
>   src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 
> 
> 
> Diff: https://reviews.apache.org/r/58303/diff/1/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 58303: Pass the slave pointer directly to Master::removeTask.

2017-04-10 Thread James Peach 

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

Review request for mesos and Mesos Reviewbot.


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


Repository: mesos


Description
---

In all cases where we call Master::removeTask, we have the correct slave
pointer in hand. We can just pass it down, avoiding the need to look it
up again with the SlaveID.


Diffs
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 


Diff: https://reviews.apache.org/r/58303/diff/1/


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach