Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

(Updated April 10, 2017, 3:50 p.m.)


Review request for mesos and Mesos Reviewbot.


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


Repository: mesos


Description
---

In a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs
-

  src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 


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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[Adam B]

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


Fix it, then Ship it!




LGTM, minor tweak to a log message


src/master/master.cpp
Lines 6429-6431 (original), 6433-6435 (patched)


s/shutdown.executor_id()/executorId/
s/shutdown.slave_id()/slaveId/
(now that you have them already)


- Adam B


On April 10, 2017, 8:50 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58304/
> ---
> 
> (Updated April 10, 2017, 8:50 a.m.)
> 
> 
> Review request for mesos and Mesos Reviewbot.
> 
> 
> Bugs: MESOS-7372
> https://issues.apache.org/jira/browse/MESOS-7372
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a number of places, we tested whether the slaves hashmap contains
> the desired element before indexing it. It is safe to just index it and
> check for a NULL result, which saves us some hashing and lookups.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 
> 
> 
> Diff: https://reviews.apache.org/r/58304/diff/1/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

(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 a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs (updated)
-

  src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 


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

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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[Neil Conway]

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



For consistency, should we also remove the 
`CHECK(slaves.registered.contains(slaveId))` in `Master::updateUnavailability`?


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


Seems a bit implicit to me, but I guess it's fine.

If we're going to make this change, can you also update the 
`slaves.registered.get()` call at line 5591 (inside `Master::reregisterSlave`) 
to move the `slave` variable inside the scope of the `if`?



src/master/master.cpp
Lines 5276 (patched)


Unintended whitespace change.



src/master/master.cpp
Line 6113 (original), 6111 (patched)


I'd prefer to remove this `CHECK_NOTNULL`, because it is implied by the 
control flow of the function. i.e., I think `CHECK_NOTNULL` is useful when it 
documents a function's preconditions, but in this case it is redundant with the 
function's internal control flow.



src/master/master.cpp
Line 6381 (original), 6380 (patched)


Ibid.



src/master/master.cpp
Line 6439 (original), 6440 (patched)


Ibid.



src/master/master.cpp
Line 6986 (original), 6988 (patched)


Ibid.



src/master/master.cpp
Line 7131 (original), 7134 (patched)


Ibid.


- 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/58304/
> ---
> 
> (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 a number of places, we tested whether the slaves hashmap contains
> the desired element before indexing it. It is safe to just index it and
> check for a NULL result, which saves us some hashing and lookups.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 
> 
> 
> Diff: https://reviews.apache.org/r/58304/diff/2/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

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


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


Changes
---

Rebased and addresses review feedback.


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


Repository: mesos


Description
---

In a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs (updated)
-

  src/master/master.cpp c1f3c4f3bbcbfb10ae8fc974bd4a16ec070a79fd 


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

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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

(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 a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs (updated)
-

  src/master/master.cpp 0f4c64c6b102ef201779a331c96b5d78a98281ad 


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

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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[Neil Conway]

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


Ship it!




Ship It!

- 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/58304/
> ---
> 
> (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 a number of places, we tested whether the slaves hashmap contains
> the desired element before indexing it. It is safe to just index it and
> check for a NULL result, which saves us some hashing and lookups.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 0f4c64c6b102ef201779a331c96b5d78a98281ad 
> 
> 
> Diff: https://reviews.apache.org/r/58304/diff/4/
> 
> 
> Testing
> ---
> 
> Make check (Fedora 25). Internal fuzzing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

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


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

In a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs (updated)
-

  src/master/master.cpp 0f4c64c6b102ef201779a331c96b5d78a98281ad 


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

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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

(Updated April 19, 2017, 10:05 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 a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs (updated)
-

  src/master/master.cpp 52de2f91bdacf46f913c27382ad50b4f278ad297 


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

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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

(Updated April 19, 2017, 10:07 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 a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs
-

  src/master/master.cpp 52de2f91bdacf46f913c27382ad50b4f278ad297 


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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach

Re: Review Request 58304: Remove unnecessary hashmap lookups.

[James Peach]

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

(Updated April 19, 2017, 10:48 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 a number of places, we tested whether the slaves hashmap contains
the desired element before indexing it. It is safe to just index it and
check for a NULL result, which saves us some hashing and lookups.


Diffs (updated)
-

  src/master/master.cpp 52de2f91bdacf46f913c27382ad50b4f278ad297 


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

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


Testing
---

Make check (Fedora 25). Internal fuzzing.


Thanks,

James Peach