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 2015/08/07 15:35:09 UTC
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/
-----------------------------------------------------------
(Updated Aug. 7, 2015, 1:35 p.m.)
Review request for Aurora and Maxim Khutornenko.
Changes
-------
Updated to include proper handling of reentrant calls to `DbStorage#write()`.
Summary (updated)
-----------------
DbStorage: avoid flushing for reentrant writes, remove extra @Transactional.
Bugs: AURORA-1395
https://issues.apache.org/jira/browse/AURORA-1395
Repository: aurora
Description (updated)
-------
Two loose ends from r/37049:
- An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
- No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
Diff: https://reviews.apache.org/r/37141/diff/
Testing
-------
Thanks,
Bill Farner
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Aug. 7, 2015, 10:22 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 143
> > <https://reviews.apache.org/r/37141/diff/1-2/?file=1032851#file1032851line143>
> >
> > Hmm, can we use LogStorage writeLock.getHoldCount() instead? It could potentially wrap MutateWork into a new anonymous instance pushing lock count downstream. Or would it be too much?
>
> Bill Farner wrote:
> For what purpose? That seems like a pretty significant encapsulation violation, and couples this implementation to LogStorage where it is otherwise independent.
It's just that ThreadLocal opens up a new chapter (and not a good one) in our design patterns. I agree MutateWork wrapping isn't ideal either but it would at least consolidate around the synchronization point. Anyway, just a thought, I don't feel strongly about it.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/#review94585
-----------------------------------------------------------
On Aug. 7, 2015, 1:35 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37141/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:35 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Two loose ends from r/37049:
> - An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
> - No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
> src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
>
> Diff: https://reviews.apache.org/r/37141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 7, 2015, 10:22 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 143
> > <https://reviews.apache.org/r/37141/diff/1-2/?file=1032851#file1032851line143>
> >
> > Hmm, can we use LogStorage writeLock.getHoldCount() instead? It could potentially wrap MutateWork into a new anonymous instance pushing lock count downstream. Or would it be too much?
For what purpose? That seems like a pretty significant encapsulation violation, and couples this implementation to LogStorage where it is otherwise independent.
> On Aug. 7, 2015, 10:22 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, lines 159-160
> > <https://reviews.apache.org/r/37141/diff/1-2/?file=1032851#file1032851line159>
> >
> > This relies on LogStorage writeLock synchronization, correct?
Yes, but not because of this. The dependence on external synchronization comes from the fact that the queue itself is not specific to the transaction.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/#review94585
-----------------------------------------------------------
On Aug. 7, 2015, 1:35 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37141/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:35 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Two loose ends from r/37049:
> - An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
> - No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
> src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
>
> Diff: https://reviews.apache.org/r/37141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/#review94585
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java (line 143)
<https://reviews.apache.org/r/37141/#comment149191>
Hmm, can we use LogStorage writeLock.getHoldCount() instead? It could potentially wrap MutateWork into a new anonymous instance pushing lock count downstream. Or would it be too much?
src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java (lines 159 - 160)
<https://reviews.apache.org/r/37141/#comment149193>
This relies on LogStorage writeLock synchronization, correct?
- Maxim Khutornenko
On Aug. 7, 2015, 1:35 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37141/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:35 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Two loose ends from r/37049:
> - An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
> - No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
> src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
>
> Diff: https://reviews.apache.org/r/37141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/#review94536
-----------------------------------------------------------
Master (8adc9bd) is red with this patch.
./build-support/jenkins/build.sh
src.test.python.apache.aurora.client.cli.plugins ..... SUCCESS
src.test.python.apache.aurora.client.cli.quota ..... SUCCESS
src.test.python.apache.aurora.client.cli.sla ..... SUCCESS
src.test.python.apache.aurora.client.cli.supdate ..... SUCCESS
src.test.python.apache.aurora.client.cli.task ..... SUCCESS
src.test.python.apache.aurora.client.cli.update ..... SUCCESS
src.test.python.apache.aurora.client.cli.version ..... SUCCESS
src.test.python.apache.aurora.client.config ..... SUCCESS
src.test.python.apache.aurora.client.hooks.hooked_api ..... SUCCESS
src.test.python.apache.aurora.client.hooks.non_hooked_api ..... SUCCESS
src.test.python.apache.aurora.common.test_aurora_job_key ..... SUCCESS
src.test.python.apache.aurora.common.test_cluster ..... SUCCESS
src.test.python.apache.aurora.common.test_cluster_option ..... SUCCESS
src.test.python.apache.aurora.common.test_clusters ..... SUCCESS
src.test.python.apache.aurora.common.test_http_signaler ..... SUCCESS
src.test.python.apache.aurora.common.test_pex_version ..... SUCCESS
src.test.python.apache.aurora.common.test_shellify ..... SUCCESS
src.test.python.apache.aurora.common.test_transport ..... SUCCESS
src.test.python.apache.aurora.config.test_base ..... SUCCESS
src.test.python.apache.aurora.config.test_constraint_parsing ..... SUCCESS
src.test.python.apache.aurora.config.test_loader ..... SUCCESS
src.test.python.apache.aurora.config.test_thrift ..... SUCCESS
src.test.python.apache.aurora.executor.common.path_detector ..... SUCCESS
src.test.python.apache.aurora.executor.common.task_info ..... SUCCESS
src.test.python.apache.aurora.executor.executor_base ..... SUCCESS
src.test.python.apache.aurora.executor.executor_vars ..... SUCCESS
src.test.python.apache.aurora.executor.http_lifecycle ..... SUCCESS
src.test.python.apache.aurora.executor.status_manager ..... SUCCESS
src.test.python.apache.aurora.executor.thermos_task_runner ..... FAILURE
src.test.python.apache.thermos.cli.commands.commands ..... SUCCESS
src.test.python.apache.thermos.cli.common ..... SUCCESS
src.test.python.apache.thermos.cli.main ..... SUCCESS
src.test.python.apache.thermos.common.test_pathspec ..... SUCCESS
src.test.python.apache.thermos.core.test_runner_integration ..... SUCCESS
src.test.python.apache.thermos.monitoring.test_disk ..... SUCCESS
FAILURE
[31m
FAILURE[0m
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 7, 2015, 1:35 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37141/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:35 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Two loose ends from r/37049:
> - An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
> - No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
> src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
>
> Diff: https://reviews.apache.org/r/37141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/#review94550
-----------------------------------------------------------
Ship it!
Master (8adc9bd) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 7, 2015, 1:35 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37141/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:35 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Two loose ends from r/37049:
> - An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
> - No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
> src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
>
> Diff: https://reviews.apache.org/r/37141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/#review94543
-----------------------------------------------------------
@ReviewBot retry
- Bill Farner
On Aug. 7, 2015, 1:35 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37141/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:35 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Two loose ends from r/37049:
> - An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
> - No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
> src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
>
> Diff: https://reviews.apache.org/r/37141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 37141: DbStorage: avoid flushing for reentrant
writes, remove extra @Transactional.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37141/#review94773
-----------------------------------------------------------
Ship it!
Ship It!
- Maxim Khutornenko
On Aug. 7, 2015, 1:35 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37141/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 1:35 p.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Two loose ends from r/37049:
> - An extra @Transactional which rendered the inner transaction useless, and results in async work still being performed within the transaction.
> - No handling for reentrant calls to write(), causing async work to be flushed in inner calls and before the transaction actually completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java aac62e2bbc212f61e61ffca75753ef06f1701ea4
> src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 3b05db9481edc7a82c0823cf50793c7d12384541
>
> Diff: https://reviews.apache.org/r/37141/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>