> On Oct. 15, 2014, 7:06 p.m., Ben Mahler wrote: > > src/state/log.cpp, lines 364-369 > > <https://reviews.apache.org/r/24536/diff/4/?file=719263#file719263line364> > > > > 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?
Yes, we can skip this check since the Snapshot::position is already the "base index" for the snapshot. I've updated the comment around Snapshot::position and also added a test just for this case. > On Oct. 15, 2014, 7:06 p.m., Ben Mahler wrote: > > src/state/log.cpp, lines 465-467 > > <https://reviews.apache.org/r/24536/diff/4/?file=719263#file719263line465> > > > > Can you expand on this a bit? Not sure what this is suggesting, > > breaking out the diff call into an async continuation? On second thought, not sure that getting more deterministic write latency is really worth it here, so I dropped the comment. > On Oct. 15, 2014, 7:06 p.m., Ben Mahler wrote: > > src/state/log.cpp, lines 491-493 > > <https://reviews.apache.org/r/24536/diff/4/?file=719263#file719263line491> > > > > Curious, if we failed to diff, should we fall back to a snapshot > > instead? I added a TODO. > On Oct. 15, 2014, 7:06 p.m., Ben Mahler wrote: > > src/state/log.cpp, lines 535-545 > > <https://reviews.apache.org/r/24536/diff/4/?file=719263#file719263line535> > > > > 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. I personally prefer the Operation being filled in "close" to where we actually serialize. It's only a couple of extra lines! > On Oct. 15, 2014, 7:06 p.m., Ben Mahler wrote: > > src/state/log.cpp, lines 305-310 > > <https://reviews.apache.org/r/24536/diff/4/?file=719263#file719263line305> > > > > 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()); > > ``` Why thank you! > On Oct. 15, 2014, 7:06 p.m., Ben Mahler wrote: > > src/Makefile.am, lines 584-588 > > <https://reviews.apache.org/r/24536/diff/4/?file=719258#file719258line584> > > > > 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. :) That's all being taken care of in a seperate review! - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24536/#review56624 ----------------------------------------------------------- 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 > >
