You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2014/02/26 00:42:33 UTC
Review Request 18487: Refactor LogStorage to more cleanly separate replay
and write-behind modes,
and require explicit implementation of mutate operations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Repository: aurora
Description
-------
There are a few nice things falling out of this refactor:
- New methods on mutable store interfaces are not implicitly forwarded
(ForwardingStore does not implement mutable stores).
- Write ahead/behind behavior is more obvious in LogStorage
(i found the delegation by calling super tough to catch mistakes.)
- Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
(they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
- the boolean recovered flag disappears, making LogStorage.write() easier to reason about
(CallOrderEnforcingStorage helps bolster this.)
Diffs
-----
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
Diff: https://reviews.apache.org/r/18487/diff/
Testing
-------
./gradlew build
Thanks,
Bill Farner
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Bill Farner <wf...@apache.org>.
> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java, line 335
> > <https://reviews.apache.org/r/18487/diff/2/?file=503866#file503866line335>
> >
> > How about extracting a private transactionManager field initialized inline? Small price for a big improvement in readability.
Done.
> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java, line 87
> > <https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line87>
> >
> > Extra space.
Fixed.
> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java, line 91
> > <https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line91>
> >
> > Typo.
Fixed.
> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java, line 158
> > <https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line158>
> >
> > This seems to be a prevailing pattern: assertWriting() followed by log(). Why not retain the previous approach of asserting inside of a log()? You could still assert earlier when needed with the assertion in log() being a catch-all case.
Thanks for catching, this was a regression due to a sequence of dependent reviews.
> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java, line 280
> > <https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line280>
> >
> > Mind converting this comment into a message arg?
Done. Also added a TODO in TaskStore to give some insight into where i'd like to see these methods go.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/#review35547
-----------------------------------------------------------
On Feb. 26, 2014, 12:12 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18487/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 12:12 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few nice things falling out of this refactor:
> - New methods on mutable store interfaces are not implicitly forwarded
> (ForwardingStore does not implement mutable stores).
> - Write ahead/behind behavior is more obvious in LogStorage
> (i found the delegation by calling super tough to catch mistakes.)
> - Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
> (they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
>
> Diff: https://reviews.apache.org/r/18487/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/#review35547
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java
<https://reviews.apache.org/r/18487/#comment66145>
How about extracting a private transactionManager field initialized inline? Small price for a big improvement in readability.
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java
<https://reviews.apache.org/r/18487/#comment66154>
Extra space.
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java
<https://reviews.apache.org/r/18487/#comment66156>
Typo.
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java
<https://reviews.apache.org/r/18487/#comment66155>
This seems to be a prevailing pattern: assertWriting() followed by log(). Why not retain the previous approach of asserting inside of a log()? You could still assert earlier when needed with the assertion in log() being a catch-all case.
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java
<https://reviews.apache.org/r/18487/#comment66157>
Mind converting this comment into a message arg?
- Maxim Khutornenko
On Feb. 26, 2014, 12:12 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18487/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2014, 12:12 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few nice things falling out of this refactor:
> - New methods on mutable store interfaces are not implicitly forwarded
> (ForwardingStore does not implement mutable stores).
> - Write ahead/behind behavior is more obvious in LogStorage
> (i found the delegation by calling super tough to catch mistakes.)
> - Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
> (they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
>
> Diff: https://reviews.apache.org/r/18487/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/
-----------------------------------------------------------
(Updated March 14, 2014, 11:24 p.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Repository: aurora
Description
-------
There are a few nice things falling out of this refactor:
- New methods on mutable store interfaces are not implicitly forwarded
(ForwardingStore does not implement mutable stores).
- Write ahead/behind behavior is more obvious in LogStorage
(i found the delegation by calling super tough to catch mistakes.)
- Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
(they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 8758eb7f2d80b88bc13d42fa62021c255a54fdc8
src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 3d0ff2dfc01404c890dc17d23c3d15732915c438
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 3ccf800a14cb987b798bc28d15dcbef50b4c78e7
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 8b26cc0ac2a2a1be6e4cde91b3f84ae00ec98d29
Diff: https://reviews.apache.org/r/18487/diff/
Testing
-------
./gradlew build
Thanks,
Bill Farner
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Bill Farner <wf...@apache.org>.
> On March 5, 2014, 6:18 p.m., Bill Farner wrote:
> > Ping? Kevin — waiting on a review from you.
Ping?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/#review36247
-----------------------------------------------------------
On March 3, 2014, 6:29 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18487/
> -----------------------------------------------------------
>
> (Updated March 3, 2014, 6:29 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few nice things falling out of this refactor:
> - New methods on mutable store interfaces are not implicitly forwarded
> (ForwardingStore does not implement mutable stores).
> - Write ahead/behind behavior is more obvious in LogStorage
> (i found the delegation by calling super tough to catch mistakes.)
> - Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
> (they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
> src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 3d0ff2dfc01404c890dc17d23c3d15732915c438
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
>
> Diff: https://reviews.apache.org/r/18487/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/#review36247
-----------------------------------------------------------
Ping? Kevin — waiting on a review from you.
- Bill Farner
On March 3, 2014, 6:29 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18487/
> -----------------------------------------------------------
>
> (Updated March 3, 2014, 6:29 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few nice things falling out of this refactor:
> - New methods on mutable store interfaces are not implicitly forwarded
> (ForwardingStore does not implement mutable stores).
> - Write ahead/behind behavior is more obvious in LogStorage
> (i found the delegation by calling super tough to catch mistakes.)
> - Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
> (they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
> src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 3d0ff2dfc01404c890dc17d23c3d15732915c438
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
>
> Diff: https://reviews.apache.org/r/18487/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/#review37275
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On March 3, 2014, 10:29 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18487/
> -----------------------------------------------------------
>
> (Updated March 3, 2014, 10:29 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> There are a few nice things falling out of this refactor:
> - New methods on mutable store interfaces are not implicitly forwarded
> (ForwardingStore does not implement mutable stores).
> - Write ahead/behind behavior is more obvious in LogStorage
> (i found the delegation by calling super tough to catch mistakes.)
> - Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
> (they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
> src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 3d0ff2dfc01404c890dc17d23c3d15732915c438
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
>
> Diff: https://reviews.apache.org/r/18487/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/
-----------------------------------------------------------
(Updated March 3, 2014, 6:29 p.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Repository: aurora
Description
-------
There are a few nice things falling out of this refactor:
- New methods on mutable store interfaces are not implicitly forwarded
(ForwardingStore does not implement mutable stores).
- Write ahead/behind behavior is more obvious in LogStorage
(i found the delegation by calling super tough to catch mistakes.)
- Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
(they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 3d0ff2dfc01404c890dc17d23c3d15732915c438
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
Diff: https://reviews.apache.org/r/18487/diff/
Testing
-------
./gradlew build
Thanks,
Bill Farner
Re: Review Request 18487: Refactor LogStorage to more cleanly separate
replay and write-behind modes,
and require explicit implementation of mutate operations.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18487/
-----------------------------------------------------------
(Updated Feb. 26, 2014, 12:12 a.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Changes
-------
Decided to backpedal on the removal of LogStorage.recovered flag, since it has a bigger ripple than i'd like to add here.
Repository: aurora
Description (updated)
-------
There are a few nice things falling out of this refactor:
- New methods on mutable store interfaces are not implicitly forwarded
(ForwardingStore does not implement mutable stores).
- Write ahead/behind behavior is more obvious in LogStorage
(i found the delegation by calling super tough to catch mistakes.)
- Callers with a handle on LogStorage don't have a means to invoke mutate calls outside of a transaction
(they only get access to mutable stores in write(), which obviated testMutateRequiresWriteOperation)
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 1842210c91ee3e153f5d142266bc2696960535af
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java d6624f470eadaf0be658db1d87d5863ea6701d94
Diff: https://reviews.apache.org/r/18487/diff/
Testing
-------
./gradlew build
Thanks,
Bill Farner