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/01/27 22:31:26 UTC
Review Request 17423: Added log implementation for state storage.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17423/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Jie Yu.
Repository: mesos-git
Description
-------
This is done in C++11, I've converted to C++03 in https://reviews.apache.org/r/17424.
Note that this does not implement diffs but that's probably okay for our initial use case. This also does not implement caching so all state entries are stored in memory, which is also okay for our initial use case. Finally, this does not implement defragmentation which again is probably okay for our use case. These are captured as TODOs in the code.
Diffs
-----
src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa
src/state/log.hpp PRE-CREATION
src/state/log.cpp PRE-CREATION
src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
src/tests/state_tests.cpp 03c538861a88d3a07e2468dce5553eeb3acc9243
Diff: https://reviews.apache.org/r/17423/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 17423: Added log implementation for state storage.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Jan. 30, 2014, 7:59 p.m., Jie Yu wrote:
> > src/state/log.cpp, line 94
> > <https://reviews.apache.org/r/17423/diff/1/?file=451877#file451877line94>
> >
> > We can kill this if the following patch is out:
> > https://reviews.apache.org/r/17476/
> >
> > Replace it with a Sequence object.
I've decided to move to an "asynchronous" mutex instead. This is really simpler to reason about (now that the discard semantics have changed) and doesn't require adding overloads to Sequence for both C++03 and C++11.
> On Jan. 30, 2014, 7:59 p.m., Jie Yu wrote:
> > src/state/log.cpp, line 297
> > <https://reviews.apache.org/r/17423/diff/1/?file=451877#file451877line297>
> >
> > Comments about why atomicity can be achieved here might be great. For example, I may wonder: is it possible that some other writer gets elected and writes the same variable ahead in the log while this writer does not know about it and somehow is writing a 'hole' in the log (and succeeds). In that case, the other writer's append may not be atomic.
> >
> > I guess one important invariant here is: when a writer successfully appends a value to the log (at position p), all values at positions prior to p should have been agreed and properly replayed (thus reflected in the in-memory snapshots).
The atomicity here is guaranteed because of the nature of starting a Log::Writer and doing an append. Now that Log::Writer demotion is exposed (via returning options) we're guaranteed to know when something that we tried to do was no longer atomic. I've added some small comments as such to the LogStorageProcess class.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17423/#review33233
-----------------------------------------------------------
On Jan. 27, 2014, 9:33 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17423/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 9:33 p.m.)
>
>
> Review request for mesos, Ben Mahler and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is done in C++11, I've converted to C++03 in https://reviews.apache.org/r/17424.
>
> Note that this does not implement diffs but that's probably okay for our initial use case. This also does not implement caching so all state entries are stored in memory, which is also okay for our initial use case. Finally, this does not implement defragmentation which again is probably okay for our use case. These are captured as TODOs in the code.
>
> Also, this uses a 'sequence' operation which will be replaced with work being done by Jie Yu.
>
>
> Diffs
> -----
>
> src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
> src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa
> src/state/log.hpp PRE-CREATION
> src/state/log.cpp PRE-CREATION
> src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
> src/tests/state_tests.cpp 03c538861a88d3a07e2468dce5553eeb3acc9243
>
> Diff: https://reviews.apache.org/r/17423/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 17423: Added log implementation for state storage.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17423/#review33233
-----------------------------------------------------------
Great stuff!
src/messages/state.proto
<https://reviews.apache.org/r/17423/#comment62599>
Kill extra lines.
src/state/log.cpp
<https://reviews.apache.org/r/17423/#comment62600>
s/startd/started/
src/state/log.cpp
<https://reviews.apache.org/r/17423/#comment62601>
We can kill this if the following patch is out:
https://reviews.apache.org/r/17476/
Replace it with a Sequence object.
src/state/log.cpp
<https://reviews.apache.org/r/17423/#comment62602>
Ditto.
src/state/log.cpp
<https://reviews.apache.org/r/17423/#comment62605>
Kill spaces.
src/state/log.cpp
<https://reviews.apache.org/r/17423/#comment62622>
Comments about why atomicity can be achieved here might be great. For example, I may wonder: is it possible that some other writer gets elected and writes the same variable ahead in the log while this writer does not know about it and somehow is writing a 'hole' in the log (and succeeds). In that case, the other writer's append may not be atomic.
I guess one important invariant here is: when a writer successfully appends a value to the log (at position p), all values at positions prior to p should have been agreed and properly replayed (thus reflected in the in-memory snapshots).
src/tests/log_tests.cpp
<https://reviews.apache.org/r/17423/#comment62623>
What's this:)
src/tests/state_tests.cpp
<https://reviews.apache.org/r/17423/#comment62627>
extends from TemporaryDirectoryTest?
src/tests/state_tests.cpp
<https://reviews.apache.org/r/17423/#comment62628>
so that you don't have to do this if TemporaryDirectoryTest is used?
- Jie Yu
On Jan. 27, 2014, 9:33 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17423/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 9:33 p.m.)
>
>
> Review request for mesos, Ben Mahler and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is done in C++11, I've converted to C++03 in https://reviews.apache.org/r/17424.
>
> Note that this does not implement diffs but that's probably okay for our initial use case. This also does not implement caching so all state entries are stored in memory, which is also okay for our initial use case. Finally, this does not implement defragmentation which again is probably okay for our use case. These are captured as TODOs in the code.
>
> Also, this uses a 'sequence' operation which will be replaced with work being done by Jie Yu.
>
>
> Diffs
> -----
>
> src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
> src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa
> src/state/log.hpp PRE-CREATION
> src/state/log.cpp PRE-CREATION
> src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
> src/tests/state_tests.cpp 03c538861a88d3a07e2468dce5553eeb3acc9243
>
> Diff: https://reviews.apache.org/r/17423/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 17423: Added log implementation for state storage.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Jan. 31, 2014, 6:02 p.m., Jie Yu wrote:
> > src/state/log.cpp, line 348
> > <https://reviews.apache.org/r/17423/diff/1/?file=451877#file451877line348>
> >
> > "writer.append" could potentially block forever if there is, for example, a network blip (i.e. the coordinator will wait for a quorum of responses from replicas, but is not able to get that due to network issue).
> >
> > In that case, the sequence of callbacks will be blocked. We may wanna introduce a timeout for writer.append/truncate (even for writer.start).
> >
> > Whenever timeout is introduced, there is a problem of async cancellation. In case of a timeout, to ensure the sequence semantics, you wanna make sure that all operations (e.g. associated with writer.append) are properly cancelled before failing the future so that the next operation will not overlap with the previous operation.
> >
> > Maybe for this case, it's fine because the underlying Paxos layer can tolerate overlapping operations and won't cause any consistency issue even if operations are indeed overlapped (very rare situation).
I've added https://reviews.apache.org/r/18954 which will make putting adding these kinds of timeouts in callers that get back future's easier, and we should strive to push the timeouts out of implementations as much as possible.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17423/#review33339
-----------------------------------------------------------
On Jan. 27, 2014, 9:33 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17423/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 9:33 p.m.)
>
>
> Review request for mesos, Ben Mahler and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is done in C++11, I've converted to C++03 in https://reviews.apache.org/r/17424.
>
> Note that this does not implement diffs but that's probably okay for our initial use case. This also does not implement caching so all state entries are stored in memory, which is also okay for our initial use case. Finally, this does not implement defragmentation which again is probably okay for our use case. These are captured as TODOs in the code.
>
> Also, this uses a 'sequence' operation which will be replaced with work being done by Jie Yu.
>
>
> Diffs
> -----
>
> src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
> src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa
> src/state/log.hpp PRE-CREATION
> src/state/log.cpp PRE-CREATION
> src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
> src/tests/state_tests.cpp 03c538861a88d3a07e2468dce5553eeb3acc9243
>
> Diff: https://reviews.apache.org/r/17423/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 17423: Added log implementation for state storage.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17423/#review33339
-----------------------------------------------------------
src/state/log.cpp
<https://reviews.apache.org/r/17423/#comment62794>
"writer.append" could potentially block forever if there is, for example, a network blip (i.e. the coordinator will wait for a quorum of responses from replicas, but is not able to get that due to network issue).
In that case, the sequence of callbacks will be blocked. We may wanna introduce a timeout for writer.append/truncate (even for writer.start).
Whenever timeout is introduced, there is a problem of async cancellation. In case of a timeout, to ensure the sequence semantics, you wanna make sure that all operations (e.g. associated with writer.append) are properly cancelled before failing the future so that the next operation will not overlap with the previous operation.
Maybe for this case, it's fine because the underlying Paxos layer can tolerate overlapping operations and won't cause any consistency issue even if operations are indeed overlapped (very rare situation).
- Jie Yu
On Jan. 27, 2014, 9:33 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17423/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2014, 9:33 p.m.)
>
>
> Review request for mesos, Ben Mahler and Jie Yu.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is done in C++11, I've converted to C++03 in https://reviews.apache.org/r/17424.
>
> Note that this does not implement diffs but that's probably okay for our initial use case. This also does not implement caching so all state entries are stored in memory, which is also okay for our initial use case. Finally, this does not implement defragmentation which again is probably okay for our use case. These are captured as TODOs in the code.
>
> Also, this uses a 'sequence' operation which will be replaced with work being done by Jie Yu.
>
>
> Diffs
> -----
>
> src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
> src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa
> src/state/log.hpp PRE-CREATION
> src/state/log.cpp PRE-CREATION
> src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
> src/tests/state_tests.cpp 03c538861a88d3a07e2468dce5553eeb3acc9243
>
> Diff: https://reviews.apache.org/r/17423/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 17423: Added log implementation for state storage.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17423/
-----------------------------------------------------------
(Updated Jan. 27, 2014, 9:33 p.m.)
Review request for mesos, Ben Mahler and Jie Yu.
Repository: mesos-git
Description (updated)
-------
This is done in C++11, I've converted to C++03 in https://reviews.apache.org/r/17424.
Note that this does not implement diffs but that's probably okay for our initial use case. This also does not implement caching so all state entries are stored in memory, which is also okay for our initial use case. Finally, this does not implement defragmentation which again is probably okay for our use case. These are captured as TODOs in the code.
Also, this uses a 'sequence' operation which will be replaced with work being done by Jie Yu.
Diffs
-----
src/Makefile.am d58b46e99e0a041cf2a26abe44bbd1504a9539c0
src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa
src/state/log.hpp PRE-CREATION
src/state/log.cpp PRE-CREATION
src/tests/log_tests.cpp e493af4f2f2435efe168d07acd267b61afd37fe4
src/tests/state_tests.cpp 03c538861a88d3a07e2468dce5553eeb3acc9243
Diff: https://reviews.apache.org/r/17423/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman