----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7655/#review15397 -----------------------------------------------------------
Ship it! Modulo comments below. src/slave/paths.hpp <https://reviews.apache.org/r/7655/#comment33141> Can you log a warning here? I think this might be useful if the symlink fails because of this. Try<Nothing> rm = os::rm(latest); if (rm.isError()) { LOG(WARNING) << "Failed to rm latest symlink '" << latest << "': " << rm.error(); } I think CHECK_SOME is also ok here: CHECK_SOME(os::rm(latest)) << "Failed to rm latest symlink '" << latest << "'"; src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33148> Can you add a string when it's discarded? s/update manager"/update manager: " s/": " + future.failure()/future.failure() s/""/"the future was discarded" src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33149> Any reason not to use the: "Failed to checkpoint..." log format? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33150> In general I think "Could not" reads well, compared to "Couldn't", or even better for these messages: "Failed to find framework..." or "Unknown framework..." src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33151> ditto src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33152> ditto my comment above for outputting discarded src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33153> s/size() == 0/empty()/ src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33154> So they'll be transitioned by other code, or? Can you add a comment / pointer to where? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment33155> ditto discarded src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/7655/#comment33157> No easy way to WAIT_UNTIL instead of sleep? src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/7655/#comment33158> Kill the extra space: s/fd =/fd =/ src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/7655/#comment33163> I think this is cleaner if you stuff the record, result inside the while loop: int updates = 0; int acks = 0; string uuid; while (true) { StatusUpdateRecord record; Result<bool> result = protobuf::read(fd.get(), &record); if (!result.isSome()) { break; } if (record.type() == StatusUpdateRecord::UPDATE) { EXPECT_EQ(TASK_RUNNING, record.update().status().state()); uuid = record.update().uuid(); updates++; } else { EXPECT_EQ(uuid, record.uuid()); acks++; } } - Ben Mahler On Dec. 14, 2012, 8:31 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7655/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2012, 8:31 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Integrated SUM into slave. > > > Diffs > ----- > > include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801 > src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66 > src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b > src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 > src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018 > src/slave/status_update_manager.hpp PRE-CREATION > src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed > src/tests/status_update_manager_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/7655/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
