----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review16888 -----------------------------------------------------------
Ship it! src/common/protobuf_utils.hpp <https://reviews.apache.org/r/7212/#comment35834> s/reason/message/ src/common/protobuf_utils.hpp <https://reviews.apache.org/r/7212/#comment35833> None(). src/common/type_utils.hpp <https://reviews.apache.org/r/7212/#comment35835> Each argument on newline please. src/common/type_utils.hpp <https://reviews.apache.org/r/7212/#comment35836> Ditto. src/common/type_utils.hpp <https://reviews.apache.org/r/7212/#comment35838> I'd love to kill this if it's not being used. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35847> If open does not get called from outside constructor, we should make this private. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35842> Methods in StatusUpdateStream should really be returning a Try (or Result if necessary). You can use the technique of an 'Option<bool> error' to capture when an error might have occurred and then return the error when any calls are made after an error has occured (i.e., a call to handle). src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35841> Wrap path in single quotes. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35848> Likewise here ... if this is private to the implementation let's make it so. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35849> This guy only really makes sense if open/close are private and your semantics are close only gets called via the destructor. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35850> None() (here and everywhere else ... maybe just do one more commit at the end that takes care of this). src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35851> Single quotes. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35852> Return an error or make a big todo at the top of this class to do this. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment35853> s/re-send/resend/ - Benjamin Hindman On Feb. 19, 2013, 7:59 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7212/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2013, 7:59 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > added shutdownFramework > > > Minor fixes for open > > > Ben's comments > > > Refactoring SUM > > > Bens' comments > > > Status Update Manager > > Rebased off latest trunk > > Conflicts: > src/Makefile.am > src/common/protobuf_utils.hpp > src/common/utils.hpp > src/slave/slave.cpp > src/tests/protobuf_io_tests.cpp > src/tests/utils_tests.cpp > > > Diffs > ----- > > src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66 > src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a > src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00 > src/local/local.cpp 3402029e07341212717e346a97e7cd30fa52ed51 > src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f > src/slave/status_update_manager.hpp PRE-CREATION > src/slave/status_update_manager.cpp PRE-CREATION > third_party/libprocess/include/process/timeout.hpp > cac996070359a3e7ecdd8077af83c8c4cf9735fd > > Diff: https://reviews.apache.org/r/7212/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Vinod Kone > >
