> 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? > > Vinod Kone wrote: > 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?
As discussed offline, you're going to go with CHECKs correct? > 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? > > Vinod Kone wrote: > No. We currently don't. See the above comment. I think failing fast is > better here than adding that complexity. Agreed. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7212/#review14306 ----------------------------------------------------------- On Dec. 12, 2012, 11:11 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7212/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2012, 11:11 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/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 > >
