You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/08/11 04:31:45 UTC
Re: Review Request 24536: Added DIFF to the replicated log state storage
implementation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/
-----------------------------------------------------------
(Updated Aug. 11, 2014, 2:31 a.m.)
Review request for mesos, Connor Doyle and Tobi Knaup.
Repository: mesos-git
Description
-------
See summary.
Note that this hard codes the location of the subversion and Apache Portable Runtime (APR) headers.
Diffs (updated)
-----
src/Makefile.am 39af0365e429b8d08addadb09ee18080a19625f8
src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad
src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a
Diff: https://reviews.apache.org/r/24536/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 1, 2014, 5:45 p.m., Jie Yu wrote:
> > src/state/log.cpp, lines 363-367
> > <https://reviews.apache.org/r/24536/diff/3/?file=708156#file708156line363>
> >
> > Could you please add a comment about why you wanna skip snapshots with diffs (it's not obvious to me)?
> >
> > Also, could you also comment on why this won't cause garbage collection issue? For example, what if all snapshots have diffs > 0?
I updated the comment and added a TODO to discuss how to introduce compaction and defragmentation.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/#review55028
-----------------------------------------------------------
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
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Timothy Chen <tn...@apache.org>.
> On Oct. 1, 2014, 5:45 p.m., Jie Yu wrote:
> > src/Makefile.am, lines 90-91
> > <https://reviews.apache.org/r/24536/diff/3/?file=708154#file708154line90>
> >
> > Ditto my comments on the other review. You might wanna modify configure.ac.
Hi Jie, I have a patch after this that changes the configure.ac.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/#review55028
-----------------------------------------------------------
On Sept. 29, 2014, 12:45 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24536/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 12:45 p.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 27c42dfde45a449750132e416b4eaf776f8c5e3b
> src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad
> src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a
>
> Diff: https://reviews.apache.org/r/24536/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/#review55028
-----------------------------------------------------------
LGTM. The only concern I have is about the truncate() (see my comments).
I'll give a shipit once the configure.ac issue is resolved.
src/Makefile.am
<https://reviews.apache.org/r/24536/#comment95363>
Ditto my comments on the other review. You might wanna modify configure.ac.
src/state/log.cpp
<https://reviews.apache.org/r/24536/#comment95452>
Could you please add a comment about why you wanna skip snapshots with diffs (it's not obvious to me)?
Also, could you also comment on why this won't cause garbage collection issue? For example, what if all snapshots have diffs > 0?
- Jie Yu
On Sept. 29, 2014, 12:45 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24536/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 12:45 p.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 27c42dfde45a449750132e416b4eaf776f8c5e3b
> src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad
> src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a
>
> Diff: https://reviews.apache.org/r/24536/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 10, 2014, 10:30 p.m., Ben Mahler wrote:
> > Can writing DIFFs be optional via the construcotr? I'm hoping to initially leave this off for the Registrar for two reasons:
> >
> > (1) Upgrading. It would be nice to be able to upgrade the masters seamlessly with respect to the Registrar:
> >
> > Phase 1: Masters can read DIFFs, don't write DIFFs.
> > Phase 2: Masters write diffs.
> >
> > If we move directly to phase 2 (as per this patch), then the upgrade of the masters needs to be done in lock-step to ensure that an old leading master doesn't come up when there are DIFF entries written, causing unnecessary failovers. The constructor argument would give us control over this, while still making it immediately available to users of LogStorage.
> >
> > (2) Ability to disable diffs. We need to run the Registrar benchmarks for DIFF enabled LogStorage. However, we'd still like to be able to flip it off should we encounter issues in a production environment. Without a flag + constructor argument, we have to roll back to disable it.
> >
> > Sound reasonable?
Yes. I refactored DIFFS_BETWEEN_SNAPSHOTS from a constant to a value that can be passed to the constructor. The default is currently 0, i.e., always construct snapshots.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/#review56243
-----------------------------------------------------------
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
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/#review56243
-----------------------------------------------------------
Can writing DIFFs be optional via the construcotr? I'm hoping to initially leave this off for the Registrar for two reasons:
(1) Upgrading. It would be nice to be able to upgrade the masters seamlessly with respect to the Registrar:
Phase 1: Masters can read DIFFs, don't write DIFFs.
Phase 2: Masters write diffs.
If we move directly to phase 2 (as per this patch), then the upgrade of the masters needs to be done in lock-step to ensure that an old leading master doesn't come up when there are DIFF entries written, causing unnecessary failovers. The constructor argument would give us control over this, while still making it immediately available to users of LogStorage.
(2) Ability to disable diffs. We need to run the Registrar benchmarks for DIFF enabled LogStorage. However, we'd still like to be able to flip it off should we encounter issues in a production environment. Without a flag + constructor argument, we have to roll back to disable it.
Sound reasonable?
- Ben Mahler
On Sept. 29, 2014, 12:45 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24536/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 12:45 p.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 27c42dfde45a449750132e416b4eaf776f8c5e3b
> src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad
> src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a
>
> Diff: https://reviews.apache.org/r/24536/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> 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
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Oct. 18, 2014, 9:42 p.m., Ben Mahler wrote:
> > src/state/log.cpp, lines 370-373
> > <https://reviews.apache.org/r/24536/diff/5/?file=725137#file725137line370>
> >
> > 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.
This is still a very valid TODO, we'll just always have a minimum, so I updated the first sentence.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/#review57282
-----------------------------------------------------------
On Oct. 25, 2014, 11:18 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24536/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2014, 11:18 p.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 2617f77b757cb7414889520c88b1bc203dedef09
> 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
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/
-----------------------------------------------------------
(Updated Oct. 25, 2014, 11:18 p.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 (updated)
-----
src/Makefile.am 2617f77b757cb7414889520c88b1bc203dedef09
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
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/
-----------------------------------------------------------
(Updated Sept. 29, 2014, 12:45 p.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 27c42dfde45a449750132e416b4eaf776f8c5e3b
src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad
src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a
Diff: https://reviews.apache.org/r/24536/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 24536: Added DIFF to the replicated log state
storage implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/
-----------------------------------------------------------
(Updated Sept. 29, 2014, 12:24 p.m.)
Review request for mesos, Connor Doyle and Tobi Knaup.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
See summary.
Note that this hard codes the location of the subversion and Apache Portable Runtime (APR) headers.
Diffs (updated)
-----
src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b
src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad
src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a
Diff: https://reviews.apache.org/r/24536/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 24536: Added DIFF to the replicated log state storage
implementation.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24536/
-----------------------------------------------------------
(Updated Aug. 11, 2014, 3:34 a.m.)
Review request for mesos, Connor Doyle and Tobi Knaup.
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 39af0365e429b8d08addadb09ee18080a19625f8
src/messages/state.proto 59276e55fcbebdb754c20d39b13b402fd11c3dad
src/state/log.cpp fd8b28a0b5d14f5ba3e6fde4695f6d09acf9c56a
Diff: https://reviews.apache.org/r/24536/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman