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/19 01:55:38 UTC
Review Request 18241: Reworked LogStorage to avoid unnecessary extra calls
to write().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Bugs: AURORA-134
https://issues.apache.org/jira/browse/AURORA-134
Repository: aurora
Description
-------
Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
Diff: https://reviews.apache.org/r/18241/diff/
Testing
-------
./gradlew build
Thanks,
Bill Farner
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Bill Farner <wf...@apache.org>.
> On Feb. 19, 2014, 5:13 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java, line 539
> > <https://reviews.apache.org/r/18241/diff/1/?file=497164#file497164line539>
> >
> > This is too easy to forget. Any chance this assert could be moved into log() instead?
A marvelous suggestion, done!
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review34871
-----------------------------------------------------------
On Feb. 19, 2014, 12:55 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 12:55 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 19, 2014, 5:13 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java, line 539
> > <https://reviews.apache.org/r/18241/diff/1/?file=497164#file497164line539>
> >
> > This is too easy to forget. Any chance this assert could be moved into log() instead?
>
> Bill Farner wrote:
> A marvelous suggestion, done!
>
> Bill Farner wrote:
> Scratch that, moving the assertion causes the in-memory store to be mutated. I can't think of a better option for now.
>
> Bill Farner wrote:
> Maxim politely called me crazy offline, and he was correct. The above comment was a lie, and original suggestion is indeed marvelous.
Misinterpreting the facts, I was just confused :)
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review34871
-----------------------------------------------------------
On Feb. 21, 2014, 1:16 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 1:16 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Bill Farner <wf...@apache.org>.
> On Feb. 19, 2014, 5:13 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java, line 539
> > <https://reviews.apache.org/r/18241/diff/1/?file=497164#file497164line539>
> >
> > This is too easy to forget. Any chance this assert could be moved into log() instead?
>
> Bill Farner wrote:
> A marvelous suggestion, done!
Scratch that, moving the assertion causes the in-memory store to be mutated. I can't think of a better option for now.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review34871
-----------------------------------------------------------
On Feb. 19, 2014, 12:55 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 12:55 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Bill Farner <wf...@apache.org>.
> On Feb. 19, 2014, 5:13 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java, line 539
> > <https://reviews.apache.org/r/18241/diff/1/?file=497164#file497164line539>
> >
> > This is too easy to forget. Any chance this assert could be moved into log() instead?
>
> Bill Farner wrote:
> A marvelous suggestion, done!
>
> Bill Farner wrote:
> Scratch that, moving the assertion causes the in-memory store to be mutated. I can't think of a better option for now.
Maxim politely called me crazy offline, and he was correct. The above comment was a lie, and original suggestion is indeed marvelous.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review34871
-----------------------------------------------------------
On Feb. 19, 2014, 12:55 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 12:55 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review34871
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java
<https://reviews.apache.org/r/18241/#comment65254>
This is too easy to forget. Any chance this assert could be moved into log() instead?
- Maxim Khutornenko
On Feb. 19, 2014, 12:55 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2014, 12:55 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review35126
-----------------------------------------------------------
Ship it!
Ship It!
- Maxim Khutornenko
On Feb. 21, 2014, 1:16 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 1:16 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review35453
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On Feb. 20, 2014, 5:16 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 20, 2014, 5:16 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/#review35322
-----------------------------------------------------------
Ping — Kevin, waiting on a review from you.
- Bill Farner
On Feb. 21, 2014, 1:16 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18241/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2014, 1:16 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: AURORA-134
> https://issues.apache.org/jira/browse/AURORA-134
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
>
> I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
> src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
>
> Diff: https://reviews.apache.org/r/18241/diff/
>
>
> Testing
> -------
>
> ./gradlew build
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 18241: Reworked LogStorage to avoid unnecessary extra
calls to write().
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18241/
-----------------------------------------------------------
(Updated Feb. 21, 2014, 1:16 a.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Bugs: AURORA-134
https://issues.apache.org/jira/browse/AURORA-134
Repository: aurora
Description
-------
Don't unnecessarily invoke write() within LogStorage, and assert that a transaction is open when a mutation is performed.
I'm starting to explore a refactor that further cleans this up by extracting the *Store implementations out of LogStorage, which should simplify things by separating transaction management from delegation.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 36e4082acceb3045ef92a5957b500e407bfc1247
src/main/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 005bad2c6b9f13e3438f3136e63fe777c6b42507
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java f708ab3c2fa5d50b084e037f9cc0cb998935120a
Diff: https://reviews.apache.org/r/18241/diff/
Testing
-------
./gradlew build
Thanks,
Bill Farner