----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review14306 -----------------------------------------------------------
src/common/type_utils.hpp <https://reviews.apache.org/r/7212/#comment30460> If you want to clean these up: return stream << frameworkId.value(); here and below in the others. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30461> This is a unique @return formatting. Was there a reason for this? Can you make it consistent with those in cgroups_isolation_module.hpp and the like? i.e. // @ return Whether the update is handled successfully (e.g. checkpointed) src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30462> ditto src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30463> s/This would also stop re-trying any/This also stops the re-trying of any src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30464> s/of framework/of a framework src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30465> s/life time/lifetime src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30469> Looks to me like you would fail your CHECK in checkpoint below when open fails, so why not check the result here? src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30468> Seems more appropriate as a Try<Nothing> src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30506> CHECK_SOME src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30508> CHECK_SOME src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30510> Why not log an error when close fails? src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30516> bool -> Try<Nothing>? src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30512> CHECK_SOME src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30515> There's no harm, but we should be using CopyFrom here. I've seen us use MergeFrom a lot where it doesn't make sense. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment30517> kill this newline src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment30528> CHECK_SOME src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment30523> Would it be useful to include the slave id here? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment30524> In all of these cases where you return false, does this stream stall? In that, you don't consider the next update? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment30522> why are you using CHECK_NOTNULL? if new fails it will throw an std::bad_alloc, right? - Ben Mahler On Dec. 10, 2012, 7:58 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7212/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2012, 7:58 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 b2d1edf140797c7150cb4644d323296965c4f000 > src/common/protobuf_utils.hpp 69baab78c8db2d2d33ffbbe7f5e5fc80d65b0e1a > src/common/type_utils.hpp fde69aeec403b3455839dca6b0b2e1507d81ba00 > 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 > >
