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 58304: Remove unnecessary hashmap lookups.
Date Tue, 11 Apr 2017 19:22:46 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/58304/#comment244573>

    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)
<https://reviews.apache.org/r/58304/#comment244568>

    Unintended whitespace change.



src/master/master.cpp
Line 6113 (original), 6111 (patched)
<https://reviews.apache.org/r/58304/#comment244557>

    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)
<https://reviews.apache.org/r/58304/#comment244559>

    Ibid.



src/master/master.cpp
Line 6439 (original), 6440 (patched)
<https://reviews.apache.org/r/58304/#comment244560>

    Ibid.



src/master/master.cpp
Line 6986 (original), 6988 (patched)
<https://reviews.apache.org/r/58304/#comment244561>

    Ibid.



src/master/master.cpp
Line 7131 (original), 7134 (patched)
<https://reviews.apache.org/r/58304/#comment244562>

    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
> 
>


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