> On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, line 68 > > <https://reviews.apache.org/r/7212/diff/7/?file=236514#file236514line68> > > > > 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)
There was a discussion about changing the doc style in cgroups. But, I will stick with the same style as cgroups for now, so that we can change it in bulk later, if needed. > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, line 117 > > <https://reviews.apache.org/r/7212/diff/7/?file=236514#file236514line117> > > > > Looks to me like you would fail your CHECK in checkpoint below when > > open fails, so why not check the result here? My original thought process was that failure to checkpoint shouldn't kill the status update manager / slave, because there might be non-checkpointing frameworks. But, thinking a bit more about it, a failure to checkpoint seems like a fatal enough state. Since we control the paths, the only reason a checkpoint fails is because there is some sort of disk failure or permissions issue. In either case, failing fast (CHECK) seems good. I'm inclined towards this mode, because it would also make the code cleaner and simple. Thoughts? > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, line 131 > > <https://reviews.apache.org/r/7212/diff/7/?file=236514#file236514line131> > > > > CHECK_SOME fixed, here and everywhere else > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.cpp, line 214 > > <https://reviews.apache.org/r/7212/diff/7/?file=236515#file236515line214> > > > > Would it be useful to include the slave id here? I think its redundant because this is going to be on that slave? > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.cpp, line 303 > > <https://reviews.apache.org/r/7212/diff/7/?file=236515#file236515line303> > > > > why are you using CHECK_NOTNULL? if new fails it will throw an > > std::bad_alloc, right? you are right. i think its redundant. > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, line 129 > > <https://reviews.apache.org/r/7212/diff/7/?file=236514#file236514line129> > > > > Seems more appropriate as a Try<Nothing> n/a > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.cpp, line 250 > > <https://reviews.apache.org/r/7212/diff/7/?file=236515#file236515line250> > > > > In all of these cases where you return false, does this stream stall? > > > > In that, you don't consider the next update? No. We currently don't. See the above comment. I think failing fast is better here than adding that complexity. > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, line 160 > > <https://reviews.apache.org/r/7212/diff/7/?file=236514#file236514line160> > > > > bool -> Try<Nothing>? changed to void instead. > On Dec. 12, 2012, 12:36 a.m., Ben Mahler wrote: > > src/slave/status_update_manager.hpp, line 150 > > <https://reviews.apache.org/r/7212/diff/7/?file=236514#file236514line150> > > > > Why not log an error when close fails? done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review14306 ----------------------------------------------------------- On Dec. 12, 2012, 8:48 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7212/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2012, 8:48 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 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 > >
