----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24536/#review57282 -----------------------------------------------------------
Ship it! Looks good, mostly some cleanup below. I think we can have a simpler `__set` while still keeping operation filled in close as you mentioned, let me know what you think. Would be great to have a test that ensures that the boundary condition works (that we don't write another diff when we reach the limit). src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97838> This comment looks like a leftover from the bug in the last diff? We won't find a minimum now, only when there are no shapshots. src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97841> I think we can simplify `__set` while __still__ keeping the operation filled in close to serialization, consider the following structure: ``` Option<Snapshot> snapshot = snapshots.get(entry.get().name); // Check the version if we have a snapshot already. if (snapshot.isSome() && UUID::fromBytes(snapshot.get().entry.uuid()) != uuid) { return false; } // Check if we should try to compute a diff. if (snapshot.isSome() && snapshot.get().diffs < diffsBetweenSnapshots) { metrics.diff.start(); Try<svn::Diff> diff = svn::diff( snapshot.get().entry.value(), entry.value()); Duration elapsed = metrics.diff.stop(); if (diff.isError()) { return Failure(...); } VLOG(1) << ...; // Only write a DIFF if it provides a reduction in size. if (diff.get().data.size() < entry.value().size()) { Operation operation; ... return writer.append(value) .then(...); } } // Write the full snapshot. Operation operation; ... return writer.append(value) .then(...); ``` Think about it! :) src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97839> >= ? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97840> s/Milliseconds/Duration/ ? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97842> This reads backwards to me, since I think of this as "the diff is bigger than the original entry", should the conditional be flipped? src/tests/state_tests.cpp <https://reviews.apache.org/r/24536/#comment97837> ASSERT_EQ or EXPECT_EQ instead of using ==? Seems like the test should not proceed when it's non-empty? - Ben Mahler On Oct. 18, 2014, 4:59 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24536/ > ----------------------------------------------------------- > > (Updated Oct. 18, 2014, 4:59 a.m.) > > > Review request for mesos, Ben Mahler and Jie Yu. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > Note that this hard codes the location of the subversion and Apache Portable > Runtime (APR) headers. > > > Diffs > ----- > > src/Makefile.am d503c8df73cda15a9d59254e8265e4a5d0e003a4 > src/java/jni/org_apache_mesos_state_LogState.cpp PRE-CREATION > src/java/src/org/apache/mesos/state/LogState.java PRE-CREATION > src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad > src/state/log.hpp 6bd054fcd1cf79a2ad1a59da59c9a903cb25882f > src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a > src/tests/state_tests.cpp 0948b9fb7d1c378f8c90abc85b34773a6963d91f > > Diff: https://reviews.apache.org/r/24536/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
