----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24536/#review56624 -----------------------------------------------------------
Looks pretty good, but I think there is an issue with the minimum position calculation. Also curious if `__set` can be cleaned up to avoid all the redundant serialization and return paths. src/Makefile.am <https://reviews.apache.org/r/24536/#comment97009> Could you update docs/getting-started.md in this change to reflect what users on Ubuntu 12.04 and CentOS 6.5 addtionally need to install, if anything? Would be great to keep that up-to-date. :) src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97140> This looks a bit redudant, since the caller knows it is calling 'patch', this caller would end up double logging: ``` LOG(ERROR) << "Failed to patch ...": patch.error(); ``` How about just returning the patch.error()? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97150> Milliseconds are a good unit for diff time? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97160> Since you're calling .get() anyway, why not use the Option? Before: ``` CHECK(snapshots.contains(operation.diff().entry().name())); Try<Snapshot> snapshot = snapshots.get(operation.diff().entry().name()).get().patch( entry.position, operation.diff()); ``` After: ``` Option<Snapshot> snapshot = snapshots.get(operation.diff().entry().name()); CHECK_SOME(snapshot); Try<Snapshot> patched = snapshot.get().patch(entry.position, operation.diff()); ``` src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97166> This appears to be incorrect: Say we have 2 Snapshots: A: (diff=0, position=11) B: (diff=3, position=10) Since we skip the diffs here, we'll produce a minimum of position 11, something I'm missing? Don't we need to keep track of the base index for a diff'ed Snapshot to determine the correct minimum? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97168> Can you expand on this a bit? Not sure what this is suggesting, breaking out the diff call into an async continuation? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97178> Curious, if we failed to diff, should we fall back to a snapshot instead? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97176> Can you collapse these two lines? src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97170> Have you looked at this in a log? It looks like this will always be 0 * 100.0 given integer division is occurring within the parens. src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97175> Can you collapse this? ``` << "% the original size (" << Bytes(entry.value().size()) << ")"; ``` src/state/log.cpp <https://reviews.apache.org/r/24536/#comment97174> I'm curious whether we can clean up all the redundant serialization and return code in this method. Could the end of this code be the only place that deals with the serialization and return? ``` Operation operation; size_t diffs = 0; ... string value; if (!operation.SerializeToString(&value)) { return Failure(string("Failed to serialize ") + Operation::Type_Name(operation.type()) + " Operation"); } return writer.append(value) .then(defer(self(), &Self::___set, entry, diffs, lambda::_1); ``` Hoping we can avoid 4 locations here where we do the same serialization and return. - Ben Mahler On Oct. 12, 2014, 5:14 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24536/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2014, 5:14 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 > > Diff: https://reviews.apache.org/r/24536/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
