----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review17166 -----------------------------------------------------------
src/common/type_utils.hpp <https://reviews.apache.org/r/7212/#comment36383> can you quote these ids, given we currently don't enforce no whitespace? src/messages/messages.proto <https://reviews.apache.org/r/7212/#comment36365> Hm.. how is sequence used? Is it perpetually incremented across recoveries? If so let's make this a uint64. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36375> s/Whether the update is handled successfully (e.g. checkpointed)./A Future of the update operation./ You can then describe more return semantics if you like separately but I think if we want to use annotations like @return we should actually describe what we're returning in the first sentence (which is what Jie originally did before I refactored his return types). Whether sounds like a boolean to me, I think this pattern is present in the cgroups isolation module because the functions there used to return booleans before I refactored them to return Try's. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36377> ditto src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36379> Can you add a ticket or review to track this? If there is one. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36381> I don't see the need to make open a separate method, can you please inline it here? 1. open is confusing because it looks like a syscall at first glance. 2. it's not used anywhere else src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36382> ditto here for inlining, I think that will make this more clear and simplify the StatusUpdateStream struct as well! src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36384> Can you maybe add a comment or note on what the uuid is for and why there can be a mismatch? src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36385> If the StatusUpdateManager stream is in a failed state then yes IMO. Right now an error is indistinguishable from an empty pending queue. src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36386> Why are you not using the protobuf write error? We've lost that information here! src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36387> moving open() into the constructor obviates the need for this CHECK_SOME as well src/slave/status_update_manager.hpp <https://reviews.apache.org/r/7212/#comment36388> moving close() into the destructor obviates the need for this CHECK_SOME src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36399> Can you kill the empty definition below and s/;/ {}/ here? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36400> Looks like you can move this CHECK to be the first thing in this function. src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36402> Why are you checking the timeout is expired? Can you add a comment? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36404> s/might/will/ Why do we allow this to happen? If it's hard to prevent, please elaborate in this comment. src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36405> s/might/will src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36406> This looks more appropriate as a None() option than a default timeout. src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36407> kill the quotes on "terminal" src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36408> If you made timeout an option you could CHECK here that timeout is some when the stream is not empty, which seems like a good validation of your internal state, right? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36409> Can you make this an Option, that way you can CHECK_NOTNULL when the option is some in the caller? src/slave/status_update_manager.cpp <https://reviews.apache.org/r/7212/#comment36411> This fits on one line. third_party/libprocess/include/process/timeout.hpp <https://reviews.apache.org/r/7212/#comment36412> Ugh, I hate these double time comparisons but I'll be fixing that sometime soon :) - Ben Mahler On Feb. 23, 2013, 8:11 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7212/ > ----------------------------------------------------------- > > (Updated Feb. 23, 2013, 8:11 a.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 > >
