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/07/09 19:53:22 UTC

Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/
-----------------------------------------------------------

Review request for Aurora and Zameer Manji.


Bugs: AURORA-1386
    https://issues.apache.org/jira/browse/AURORA-1386


Repository: aurora


Description
-------

The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.


Diffs
-----

  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 

Diff: https://reviews.apache.org/r/36357/diff/


Testing
-------


Thanks,

Bill Farner

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91136
-----------------------------------------------------------

Ship it!


Master (8efcd06) 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 July 9, 2015, 5:59 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 5:59 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Bill Farner <wf...@apache.org>.

> On July 9, 2015, 6:05 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 115
> > <https://reviews.apache.org/r/36357/diff/1/?file=1003739#file1003739line115>
> >
> >     Once H2 becomes the default/only in memory storage, are we going to revisit the LOCK_MODE?

Yes.  My TODO in the newly-added test case advises this.


> On July 9, 2015, 6:05 p.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java, line 616
> > <https://reviews.apache.org/r/36357/diff/1/?file=1003740#file1003740line616>
> >
> >     Why not a lambda here?

I have not fallen into the default mode of using lambdas just yet because there are unresolved issues with checkstyle, pmd, and guice 3.0.  I've changed this one since it does not upset these tools.


> On July 9, 2015, 6:05 p.m., Zameer Manji wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java, line 630
> > <https://reviews.apache.org/r/36357/diff/1/?file=1003740#file1003740line630>
> >
> >     Why not a lambda here?

Checkstyle does not permit this.  There are several relevant open issues, for example: https://github.com/checkstyle/checkstyle/issues/577


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91132
-----------------------------------------------------------


On July 9, 2015, 5:59 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 5:59 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Zameer Manji <zm...@apache.org>.

> On July 9, 2015, 11:05 a.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 115
> > <https://reviews.apache.org/r/36357/diff/1/?file=1003739#file1003739line115>
> >
> >     Once H2 becomes the default/only in memory storage, are we going to revisit the LOCK_MODE?
> 
> Bill Farner wrote:
>     Yes.  My TODO in the newly-added test case advises this.

I think filing a ticket for this would be nice. It is good tech debt work.


- Zameer


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91132
-----------------------------------------------------------


On July 9, 2015, 11:22 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 11:22 a.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Bill Farner <wf...@apache.org>.

> On July 9, 2015, 6:05 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 115
> > <https://reviews.apache.org/r/36357/diff/1/?file=1003739#file1003739line115>
> >
> >     Once H2 becomes the default/only in memory storage, are we going to revisit the LOCK_MODE?
> 
> Bill Farner wrote:
>     Yes.  My TODO in the newly-added test case advises this.
> 
> Zameer Manji wrote:
>     I think filing a ticket for this would be nice. It is good tech debt work.

https://issues.apache.org/jira/browse/AURORA-1389


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91132
-----------------------------------------------------------


On July 9, 2015, 7:04 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 7:04 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91132
-----------------------------------------------------------



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java (line 115)
<https://reviews.apache.org/r/36357/#comment144406>

    Once H2 becomes the default/only in memory storage, are we going to revisit the LOCK_MODE?



src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java (line 616)
<https://reviews.apache.org/r/36357/#comment144407>

    Why not a lambda here?



src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java (line 630)
<https://reviews.apache.org/r/36357/#comment144409>

    Why not a lambda here?


- Zameer Manji


On July 9, 2015, 10:59 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 10:59 a.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91138
-----------------------------------------------------------

Ship it!


Ship It!

- Zameer Manji


On July 9, 2015, 11:22 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 11:22 a.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91160
-----------------------------------------------------------

Ship it!


Master (b86b293) 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 July 9, 2015, 7:04 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 7:04 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/
-----------------------------------------------------------

(Updated July 9, 2015, 7:04 p.m.)


Review request for Aurora and Zameer Manji.


Changes
-------

Fix build break.


Bugs: AURORA-1386
    https://issues.apache.org/jira/browse/AURORA-1386


Repository: aurora


Description
-------

See h2 docs [1] for context on this setting.

The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.

[1] http://www.h2database.com/html/advanced.html#transaction_isolation


Diffs (updated)
-----

  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 

Diff: https://reviews.apache.org/r/36357/diff/


Testing
-------


Thanks,

Bill Farner

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/#review91143
-----------------------------------------------------------


Master (b86b293) is red with this patch.
  ./build-support/jenkins/build.sh

:api:compileJava UP-TO-DATE
:api:generateThriftResources
:api:processResources UP-TO-DATE
:api:classes
:api:jar
:compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2

:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileJmhJavaNote: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processJmhResources UP-TO-DATE
:jmhClasses
:checkstyleJmh
:jsHint
:checkstyleMain
:compileTestJava
:processTestResources
:testClasses
:checkstyleTest[ant:checkstyle] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java:29:8: Unused import - com.google.common.testing.TearDown.
 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleTest'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/test.xml

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 2 mins 5.723 secs


I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On July 9, 2015, 6:22 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36357/
> -----------------------------------------------------------
> 
> (Updated July 9, 2015, 6:22 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1386
>     https://issues.apache.org/jira/browse/AURORA-1386
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> See h2 docs [1] for context on this setting.
> 
> The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.
> 
> [1] http://www.h2database.com/html/advanced.html#transaction_isolation
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
>   src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 
> 
> Diff: https://reviews.apache.org/r/36357/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/
-----------------------------------------------------------

(Updated July 9, 2015, 6:22 p.m.)


Review request for Aurora and Zameer Manji.


Bugs: AURORA-1386
    https://issues.apache.org/jira/browse/AURORA-1386


Repository: aurora


Description
-------

See h2 docs [1] for context on this setting.

The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.

[1] http://www.h2database.com/html/advanced.html#transaction_isolation


Diffs (updated)
-----

  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 

Diff: https://reviews.apache.org/r/36357/diff/


Testing
-------


Thanks,

Bill Farner

Re: Review Request 36357: Relax the transaction isolation in H2 to match MemStorage behavior.

Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36357/
-----------------------------------------------------------

(Updated July 9, 2015, 5:59 p.m.)


Review request for Aurora and Zameer Manji.


Bugs: AURORA-1386
    https://issues.apache.org/jira/browse/AURORA-1386


Repository: aurora


Description (updated)
-------

See h2 docs [1] for context on this setting.

The patch in `DbModule` could have been much smaller, but i did a mini refactor to supply a map of args to centralize concatenation and make for easier commenting on each arg.

[1] http://www.h2database.com/html/advanced.html#transaction_isolation


Diffs
-----

  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 23bf0acf33b7a130bec98e33259bde1b3dc5c7cb 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java c8a2d81bfc48ee8d9cb81aecea1baba920b2b948 

Diff: https://reviews.apache.org/r/36357/diff/


Testing
-------


Thanks,

Bill Farner