Re: Review Request: Slave Restart (Part 8): Recover status update manager

[Ben Mahler <be...@gmail.com>]

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

Ship it!


As a higher level comment, have you reasoned about the slave dying _during_ recovery?


src/slave/flags.hpp
<https://reviews.apache.org/r/8736/#comment36664>

    Why not make this change in the review that introduced the 'kill' in the first place? Makes this chain harder to review because changes from an earlier review on this line will get stomped on during rebase, no?
    
    Ah well, next time.



src/slave/slave.hpp
<https://reviews.apache.org/r/8736/#comment36666>

    No comment necessary since the "_" prefix pattern indicates this is a continuation of recover.



src/slave/slave.cpp
<https://reviews.apache.org/r/8736/#comment36667>

    ditto with having made these changes in the right review to make it easier on reviewers and yourself ;)



src/slave/slave.cpp
<https://reviews.apache.org/r/8736/#comment36668>

    Why not do this as the first thing in finalize?



src/slave/slave.cpp
<https://reviews.apache.org/r/8736/#comment36669>

    Ah.. I see you've already taken care of many of my comments from earlier reviews, hence making the changes in the right ones! ;)



src/slave/slave.cpp
<https://reviews.apache.org/r/8736/#comment36670>

    would be nice to have os::exists returned any errors so that we can handle those differently here



src/slave/slave.cpp
<https://reviews.apache.org/r/8736/#comment36672>

    "We would be here" seems to be false, unless you moved the comment inside the loop.
    
    How about just saying: the state is none when....



src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/8736/#comment36675>

    const method?



src/slave/status_update_manager.hpp
<https://reviews.apache.org/r/8736/#comment36673>

    ditto from earlier review, on why it would be better to not have the open / close as separate functions (rather have them inline in the only spot they are used.



src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/8736/#comment36676>

    Good call.



src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/8736/#comment36677>

    kill newline?



src/slave/status_update_manager.cpp
<https://reviews.apache.org/r/8736/#comment36678>

    newline after the CHECK


- Ben Mahler


On Feb. 23, 2013, 8:12 a.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8736/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2013, 8:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Ben Mahler.
> 
> 
> Description
> -------
> 
> Status update manager now recovers the update stream and re-sends any pending updates.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 814a6e1af2c7caf77452748dfc9e6935d8386d8f 
>   src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd 
>   src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 
>   src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd 
>   src/slave/status_update_manager.hpp PRE-CREATION 
>   src/slave/status_update_manager.cpp PRE-CREATION 
>   src/tests/slave_recovery_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/8736/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>