Re: Review Request 18487: Refactor LogStorage to more cleanly separate replay and write-behind modes, and require explicit implementation of mutate operations.

[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
> 
>