subject:"Re\: Review Request 21132\: Initial attempt at h2\/DB storage implementation \(LockStore only\)"

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread David McLaughlin


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

(Updated May 27, 2014, 11:09 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Even simple changes can break the build. Passes ./gradlew build now.


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


Repository: aurora


Description
---

Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
and creates a bridge so that the existing in-memory stores can co-exist with 
the new db stores. This allows us to incrementally replace each storage 
implementation in org.apache.aurora.scheduler.storage.mem. In this patch I have 
replaced MemLockStore with DbLockStore. 


Diffs (updated)
-

  build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
80252bd5e1a48ada7fcd99ce7b25ee13e05e0410 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
ec5323259eb115030d12e20406675ff8ca435b74 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
bbbd7dc58f3715e1adb98f2b2540a9d13127c64c 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
7fcc0726a235f90f3d12df8d7d787cd34f80f889 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
96da98976eeece6dde7187e976ec5dc8871b9032 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
1daab634d4a47064cdc1fa9dc5be133d5605ecea 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
4a7db20d45e068c954fc5c6580a9bcfdd653 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
6137f5bbf00931c0cc678f7735d77dbf32aac09a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
6f6d9a46bd78c8e96c69240be8cb9ac6d564f0f8 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
d4fe607f2881a628ac4dc017fbed02a9509f2ce2 

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


Testing
---

./gradlew clean build


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread David McLaughlin


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

(Updated May 27, 2014, 10:45 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Review feedback.


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


Repository: aurora


Description
---

Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
and creates a bridge so that the existing in-memory stores can co-exist with 
the new db stores. This allows us to incrementally replace each storage 
implementation in org.apache.aurora.scheduler.storage.mem. In this patch I have 
replaced MemLockStore with DbLockStore. 


Diffs (updated)
-

  build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
80252bd5e1a48ada7fcd99ce7b25ee13e05e0410 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
ec5323259eb115030d12e20406675ff8ca435b74 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
bbbd7dc58f3715e1adb98f2b2540a9d13127c64c 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
7fcc0726a235f90f3d12df8d7d787cd34f80f889 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
96da98976eeece6dde7187e976ec5dc8871b9032 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
1daab634d4a47064cdc1fa9dc5be133d5605ecea 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
4a7db20d45e068c954fc5c6580a9bcfdd653 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
6137f5bbf00931c0cc678f7735d77dbf32aac09a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
6f6d9a46bd78c8e96c69240be8cb9ac6d564f0f8 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
d4fe607f2881a628ac4dc017fbed02a9509f2ce2 

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


Testing
---

./gradlew clean build


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread David McLaughlin


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

(Updated May 27, 2014, 10:45 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


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


Repository: aurora


Description
---

Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
and creates a bridge so that the existing in-memory stores can co-exist with 
the new db stores. This allows us to incrementally replace each storage 
implementation in org.apache.aurora.scheduler.storage.mem. In this patch I have 
replaced MemLockStore with DbLockStore. 


Diffs
-

  build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
80252bd5e1a48ada7fcd99ce7b25ee13e05e0410 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
ec5323259eb115030d12e20406675ff8ca435b74 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
bbbd7dc58f3715e1adb98f2b2540a9d13127c64c 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
7fcc0726a235f90f3d12df8d7d787cd34f80f889 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
96da98976eeece6dde7187e976ec5dc8871b9032 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
1daab634d4a47064cdc1fa9dc5be133d5605ecea 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
4a7db20d45e068c954fc5c6580a9bcfdd653 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
6137f5bbf00931c0cc678f7735d77dbf32aac09a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
6f6d9a46bd78c8e96c69240be8cb9ac6d564f0f8 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
d4fe607f2881a628ac4dc017fbed02a9509f2ce2 

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


Testing
---

./gradlew clean build


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread Kevin Sweeney


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



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


java.util.Logger


- Kevin Sweeney


On May 27, 2014, 3:37 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 27, 2014, 3:37 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
> and creates a bridge so that the existing in-memory stores can co-exist with 
> the new db stores. This allows us to incrementally replace each storage 
> implementation in org.apache.aurora.scheduler.storage.mem. In this patch I 
> have replaced MemLockStore with DbLockStore. 
> 
> 
> Diffs
> -
> 
>   build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 80252bd5e1a48ada7fcd99ce7b25ee13e05e0410 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> ec5323259eb115030d12e20406675ff8ca435b74 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> bbbd7dc58f3715e1adb98f2b2540a9d13127c64c 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 7fcc0726a235f90f3d12df8d7d787cd34f80f889 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 96da98976eeece6dde7187e976ec5dc8871b9032 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> 1daab634d4a47064cdc1fa9dc5be133d5605ecea 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 4a7db20d45e068c954fc5c6580a9bcfdd653 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 6137f5bbf00931c0cc678f7735d77dbf32aac09a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 6f6d9a46bd78c8e96c69240be8cb9ac6d564f0f8 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> d4fe607f2881a628ac4dc017fbed02a9509f2ce2 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew clean build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread David McLaughlin


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

(Updated May 27, 2014, 10:37 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Fixes for checkstyle/pmd. 


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


Repository: aurora


Description
---

Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
and creates a bridge so that the existing in-memory stores can co-exist with 
the new db stores. This allows us to incrementally replace each storage 
implementation in org.apache.aurora.scheduler.storage.mem. In this patch I have 
replaced MemLockStore with DbLockStore. 


Diffs (updated)
-

  build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
80252bd5e1a48ada7fcd99ce7b25ee13e05e0410 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
ec5323259eb115030d12e20406675ff8ca435b74 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
bbbd7dc58f3715e1adb98f2b2540a9d13127c64c 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
7fcc0726a235f90f3d12df8d7d787cd34f80f889 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
96da98976eeece6dde7187e976ec5dc8871b9032 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
1daab634d4a47064cdc1fa9dc5be133d5605ecea 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
4a7db20d45e068c954fc5c6580a9bcfdd653 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
6137f5bbf00931c0cc678f7735d77dbf32aac09a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
6f6d9a46bd78c8e96c69240be8cb9ac6d564f0f8 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
d4fe607f2881a628ac4dc017fbed02a9509f2ce2 

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


Testing
---

./gradlew clean build


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread David McLaughlin



> On May 27, 2014, 10:12 p.m., Kevin Sweeney wrote:
> > The rebased patch does not pass checkstyle:
> > 
> > ./gradlew -Pq clean build
> > Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf8
> > :clean
> > :about
> > :bootstrapThrift UP-TO-DATE
> > :checkPython
> > :generateSources
> > :compileGeneratedJava
> > Note: Some input files use unchecked or unsafe operations.
> > Note: Recompile with -Xlint:unchecked for details.
> > :processGeneratedResources UP-TO-DATE
> > :generatedClasses
> > :compileJava
> > Note: Writing 
> > file:/home/ksweeney/workspace/aurora/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2
> > :processResources
> > :classes
> > :jar
> > :assemble
> > :jsHint
> > Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf8
> > :checkstyleMain
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java:2:
> >  Line does not match expected header line of '^ \* Licensed under the 
> > Apache License, Version 2.0 \(the "License"\);$'.
> > [ant:checkstyle] 
> > /home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java:73:
> >  First sentence should end with a period.
> > :checkstyleMain FAILED
> > 
> > FAILURE: Build failed with an exception.
> > 
> > * What went wrong:
> > Execution failed for task ':checkstyleMain'.
> > > Checkstyle rule violations were found. See the report at: 
> > > file:///home/ksweeney/workspace/aurora/dist/reports/checkstyle/main.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: 34.843 secs
> >

D'oh. Will fix. 

As an aside - I misread that discussion on the other review. Seems unintuitive 
to me that -Pq is less quick/quiet than the default. I'd much prefer ./gradlew 
build be inclusive, and then have -Pquick, etc. for fast iteration. 


- David


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


On May 27, 2014, 9:59 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 27, 2014, 9:59 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
> and creates a bridge so that the existing in-memory stores can co-exist with 
> the new db stores. This allows us to incrementally replace each storage 
> implementation in org.apache.aurora.scheduler.storage.mem. In this patch I 
> have replaced MemLockStore with DbLockStore. 
> 
> 
> Diffs
> -
> 
>   build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread Kevin Sweeney

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

The rebased patch does not pass checkstyle:

./gradlew -Pq clean build
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf8
:clean
:about
:bootstrapThrift UP-TO-DATE
:checkPython
:generateSources
:compileGeneratedJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
:processGeneratedResources UP-TO-DATE
:generatedClasses
:compileJava
Note: Writing 
file:/home/ksweeney/workspace/aurora/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2
:processResources
:classes
:jar
:assemble
:jsHint
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf8
:checkstyleMain
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java:2:
 Line does not match expected header line of '^ \* Licensed under the Apache 
License, Version 2.0 \(the "License"\);$'.
[ant:checkstyle] 
/home/ksweeney/workspace/aurora/src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java:73:
 First sentence should end with a period.
:checkstyleMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: 
> file:///home/ksweeney/workspace/aurora/dist/reports/checkstyle/main.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: 34.843 secs

- Kevin Sweeney

On May 27, 2014, 2:59 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 27, 2014, 2:59 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
> and creates a bridge so that the existing in-memory stores can co-exist with 
> the new db stores. This allows us to incrementally replace each storage 
> implementation in org.apache.aurora.scheduler.storage.mem. In this patch I 
> have replaced MemLockStore with DbLockStore. 
> 
> 
> Diffs
> -
> 
>   build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 80252bd5e1a48ada7fcd99ce7b25ee13e05e0410 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> ec5323259eb115030d12e20406675ff8ca435b74 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> bbbd7dc58f3715e1adb98f2b2540a9d13127c64c 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREA

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread David McLaughlin


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

(Updated May 27, 2014, 9:59 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Rebased.


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


Repository: aurora


Description
---

Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
and creates a bridge so that the existing in-memory stores can co-exist with 
the new db stores. This allows us to incrementally replace each storage 
implementation in org.apache.aurora.scheduler.storage.mem. In this patch I have 
replaced MemLockStore with DbLockStore. 


Diffs (updated)
-

  build.gradle 646cdd2ac7a0beef7df5d02f16b1d37defb0c109 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
80252bd5e1a48ada7fcd99ce7b25ee13e05e0410 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
ec5323259eb115030d12e20406675ff8ca435b74 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
bbbd7dc58f3715e1adb98f2b2540a9d13127c64c 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
7fcc0726a235f90f3d12df8d7d787cd34f80f889 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
96da98976eeece6dde7187e976ec5dc8871b9032 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
1daab634d4a47064cdc1fa9dc5be133d5605ecea 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
4a7db20d45e068c954fc5c6580a9bcfdd653 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
6137f5bbf00931c0cc678f7735d77dbf32aac09a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
6f6d9a46bd78c8e96c69240be8cb9ac6d564f0f8 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
d4fe607f2881a628ac4dc017fbed02a9509f2ce2 

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


Testing
---

./gradlew clean build


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-27 Thread Kevin Sweeney


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


David, would you mind rebasing this patch?

error: patch failed: build.gradle:163
error: build.gradle: patch does not apply
error: patch failed: 
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java:173
error: src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java: patch 
does not apply
error: patch failed: 
src/main/java/org/apache/aurora/scheduler/base/JobKeys.java:16
error: src/main/java/org/apache/aurora/scheduler/base/JobKeys.java: patch does 
not apply
error: patch failed: 
src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java:1
error: src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java: 
patch does not apply
error: patch failed: 
src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java:1
error: 
src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java: 
patch does not apply


- Kevin Sweeney


On May 22, 2014, 11:26 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 22, 2014, 11:26 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
> and creates a bridge so that the existing in-memory stores can co-exist with 
> the new db stores. This allows us to incrementally replace each storage 
> implementation in org.apache.aurora.scheduler.storage.mem. In this patch I 
> have replaced MemLockStore with DbLockStore. 
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew clean build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread David McLaughlin



> On May 22, 2014, 6:06 p.m., Henry Saputra wrote:
> > Could you describe a bit details on what changes submitted with the patch? 
> > Maybe it makes sense to internal folks at Twitter but not useful for new/ 
> > external people
> 
> Kevin Sweeney wrote:
> This is the byproduct of a mailing list discussion here: 
> https://mail-archives.apache.org/mod_mbox/incubator-aurora-dev/201404.mbox/%3CCAGRA8uPKh81F3eCp%2Bf7fE84%2B2m%2ByzounTEcbhNpTaUFWqFt-Xw%40mail.gmail.com%3E
> 
> Henry Saputra wrote:
> Thanks Kevin. So it is a byproduct or actual implementation of the 
> discussions? It is still useful to add more context of what happening or 
> desired behavior of the patch.

Henry, thanks for pointing this out. I've updated the description to give more 
detail on what this patch actually does. 


- David


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


On May 22, 2014, 6:26 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 22, 2014, 6:26 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
> and creates a bridge so that the existing in-memory stores can co-exist with 
> the new db stores. This allows us to incrementally replace each storage 
> implementation in org.apache.aurora.scheduler.storage.mem. In this patch I 
> have replaced MemLockStore with DbLockStore. 
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew clean build
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread David McLaughlin


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

(Updated May 22, 2014, 6:26 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Added more detail to description.


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


Repository: aurora


Description (updated)
---

Lays down the groundwork for h2/MyBatis integration in the aurora scheduler, 
and creates a bridge so that the existing in-memory stores can co-exist with 
the new db stores. This allows us to incrementally replace each storage 
implementation in org.apache.aurora.scheduler.storage.mem. In this patch I have 
replaced MemLockStore with DbLockStore. 


Diffs
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing (updated)
---

./gradlew clean build


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread Henry Saputra



> On May 22, 2014, 6:06 p.m., Henry Saputra wrote:
> > Could you describe a bit details on what changes submitted with the patch? 
> > Maybe it makes sense to internal folks at Twitter but not useful for new/ 
> > external people
> 
> Kevin Sweeney wrote:
> This is the byproduct of a mailing list discussion here: 
> https://mail-archives.apache.org/mod_mbox/incubator-aurora-dev/201404.mbox/%3CCAGRA8uPKh81F3eCp%2Bf7fE84%2B2m%2ByzounTEcbhNpTaUFWqFt-Xw%40mail.gmail.com%3E

Thanks Kevin. So it is a byproduct or actual implementation of the discussions? 
It is still useful to add more context of what happening or desired behavior of 
the patch.


- Henry


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


On May 22, 2014, 6 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 22, 2014, 6 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread Kevin Sweeney



> On May 22, 2014, 11:06 a.m., Henry Saputra wrote:
> > Could you describe a bit details on what changes submitted with the patch? 
> > Maybe it makes sense to internal folks at Twitter but not useful for new/ 
> > external people

This is the byproduct of a mailing list discussion here: 
https://mail-archives.apache.org/mod_mbox/incubator-aurora-dev/201404.mbox/%3CCAGRA8uPKh81F3eCp%2Bf7fE84%2B2m%2ByzounTEcbhNpTaUFWqFt-Xw%40mail.gmail.com%3E


- Kevin


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


On May 22, 2014, 11 a.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 22, 2014, 11 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread Henry Saputra


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


Could you describe a bit details on what changes submitted with the patch? 
Maybe it makes sense to internal folks at Twitter but not useful for new/ 
external people

- Henry Saputra


On May 22, 2014, 6 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 22, 2014, 6 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread David McLaughlin


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

(Updated May 22, 2014, 6 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Review fixes.


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread David McLaughlin


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

(Updated May 22, 2014, 6 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread David McLaughlin



> On May 21, 2014, 7:06 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 35
> > 
> >
> > relational

Fixed.


> On May 21, 2014, 7:06 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 
> > 132
> > 
> >
> > remove throws StorageException

Done.


> On May 21, 2014, 7:06 p.m., Kevin Sweeney wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, line 1
> > 
> >
> > maybe add a note here that this schema is h2-specific (possibly in the 
> > filename?)

Added an inline comment mentioning it's a h2 schema.


- David


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


On May 22, 2014, 5:59 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 22, 2014, 5:59 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-22 Thread David McLaughlin


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

(Updated May 22, 2014, 5:59 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-21 Thread Kevin Sweeney


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

Ship it!



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


relational



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


remove throws StorageException



src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql


maybe add a note here that this schema is h2-specific (possibly in the 
filename?)


- Kevin Sweeney


On May 20, 2014, 1:33 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 20, 2014, 1:33 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-20 Thread David McLaughlin



> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 46
> > 
> >
> > Reword?

Done.


> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
> We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> 
> I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
> 
>
> 
> David McLaughlin wrote:
> Just to expand on this. The key difference between just storing objects 
> on heap and having some formal storage layer like an RDBMS is that for 
> in-memory, the creation of the underlying entities is performed when I do 
> this:
> 
> 
> LockKey.job(JobsKeys.from(role, env, job).builder());
> 
> 
> For an RDBMS, we have to take this instance and fetch the existing 
> primary key from the table and/or write a new row to the DB. This logic 
> either lives inside the Storage.Mutable implementation, or it leaks out to 
> the application layer where it makes no sense for the existing on heap 
> solutions.
> 
> Maxim Khutornenko wrote:
> | We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> Any thoughts on what that design would look like? I'd rather make an 
> effort to think it through now before locking ourselves into the schema.
> 
> | LockKey.job(JobsKeys.from(role, env, job).builder());
> Understood. However, this kind of specialized syntax is currently used 
> only on the application side (SchedulerThriftInterface) where it makes 
> perfect sense with the store handling it transparently.
> 
> Setting aside this particular issue, my general concern is that we don't 
> seem to know how to handle thrift unions gracefully at this point. There are 
> a few of them in our .thrift files and I am hesitant to move forward without 
> a better story/solution around it.
> 
> David McLaughlin wrote:
> My first take on this is that I'd have a manager/mapper class to 
> represent the LockKey union, which is injected with all the mappers for the 
> underlying types and handles the logic of running the correct underlying 
> create statements based on which field is set in LockKey. It would look 
> identical to the code now, just the type changes. 
>
> 
> Maxim Khutornenko wrote:
> Any chance we could see it implemented in this CR? It would set a 
> precedent and give us a clear path towards further thrift union migration.
> 
> Bill Farner wrote:
> David and i discussed approaches to this in the first draft, and the 2 
> sane paths forward we found were 1.) patch thrift to make it possible to 
> subclass a codegen'd union class, 2.) tackle the problem with our own code 
> generation to use different, custom types for database models.  We decided 
> that we would benefit from a short-term solution so we can vet the database 
> as a whole, and proceed with (2) as it becomes imminent (which it will, soon).
> 
> Maxim Khutornenko wrote:
> Sounds like a plan. David, would you mind adding a TODO describing the 
> proposed approach (2)?

Done.


- David


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


On May 20, 2014, 8:33 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 20, 2014, 8:33 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-20 Thread David McLaughlin


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

(Updated May 20, 2014, 8:33 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Style nits. 


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-20 Thread David McLaughlin


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

(Updated May 20, 2014, 8:20 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Review feedback. I moved job-specific code outside of DbLockStore. It is now 
isolated in LockRow and LockKeyMapper and I've added TODOs to both of them to 
consider code-generation. 


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-20 Thread Maxim Khutornenko


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

Ship it!


Ship It!

- Maxim Khutornenko


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-20 Thread Maxim Khutornenko



> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
> We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> 
> I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
> 
>
> 
> David McLaughlin wrote:
> Just to expand on this. The key difference between just storing objects 
> on heap and having some formal storage layer like an RDBMS is that for 
> in-memory, the creation of the underlying entities is performed when I do 
> this:
> 
> 
> LockKey.job(JobsKeys.from(role, env, job).builder());
> 
> 
> For an RDBMS, we have to take this instance and fetch the existing 
> primary key from the table and/or write a new row to the DB. This logic 
> either lives inside the Storage.Mutable implementation, or it leaks out to 
> the application layer where it makes no sense for the existing on heap 
> solutions.
> 
> Maxim Khutornenko wrote:
> | We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> Any thoughts on what that design would look like? I'd rather make an 
> effort to think it through now before locking ourselves into the schema.
> 
> | LockKey.job(JobsKeys.from(role, env, job).builder());
> Understood. However, this kind of specialized syntax is currently used 
> only on the application side (SchedulerThriftInterface) where it makes 
> perfect sense with the store handling it transparently.
> 
> Setting aside this particular issue, my general concern is that we don't 
> seem to know how to handle thrift unions gracefully at this point. There are 
> a few of them in our .thrift files and I am hesitant to move forward without 
> a better story/solution around it.
> 
> David McLaughlin wrote:
> My first take on this is that I'd have a manager/mapper class to 
> represent the LockKey union, which is injected with all the mappers for the 
> underlying types and handles the logic of running the correct underlying 
> create statements based on which field is set in LockKey. It would look 
> identical to the code now, just the type changes. 
>
> 
> Maxim Khutornenko wrote:
> Any chance we could see it implemented in this CR? It would set a 
> precedent and give us a clear path towards further thrift union migration.
> 
> Bill Farner wrote:
> David and i discussed approaches to this in the first draft, and the 2 
> sane paths forward we found were 1.) patch thrift to make it possible to 
> subclass a codegen'd union class, 2.) tackle the problem with our own code 
> generation to use different, custom types for database models.  We decided 
> that we would benefit from a short-term solution so we can vet the database 
> as a whole, and proceed with (2) as it becomes imminent (which it will, soon).

Sounds like a plan. David, would you mind adding a TODO describing the proposed 
approach (2)?


- Maxim


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


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/sto

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-19 Thread Bill Farner



> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
> We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> 
> I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
> 
>
> 
> David McLaughlin wrote:
> Just to expand on this. The key difference between just storing objects 
> on heap and having some formal storage layer like an RDBMS is that for 
> in-memory, the creation of the underlying entities is performed when I do 
> this:
> 
> 
> LockKey.job(JobsKeys.from(role, env, job).builder());
> 
> 
> For an RDBMS, we have to take this instance and fetch the existing 
> primary key from the table and/or write a new row to the DB. This logic 
> either lives inside the Storage.Mutable implementation, or it leaks out to 
> the application layer where it makes no sense for the existing on heap 
> solutions.
> 
> Maxim Khutornenko wrote:
> | We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> Any thoughts on what that design would look like? I'd rather make an 
> effort to think it through now before locking ourselves into the schema.
> 
> | LockKey.job(JobsKeys.from(role, env, job).builder());
> Understood. However, this kind of specialized syntax is currently used 
> only on the application side (SchedulerThriftInterface) where it makes 
> perfect sense with the store handling it transparently.
> 
> Setting aside this particular issue, my general concern is that we don't 
> seem to know how to handle thrift unions gracefully at this point. There are 
> a few of them in our .thrift files and I am hesitant to move forward without 
> a better story/solution around it.
> 
> David McLaughlin wrote:
> My first take on this is that I'd have a manager/mapper class to 
> represent the LockKey union, which is injected with all the mappers for the 
> underlying types and handles the logic of running the correct underlying 
> create statements based on which field is set in LockKey. It would look 
> identical to the code now, just the type changes. 
>
> 
> Maxim Khutornenko wrote:
> Any chance we could see it implemented in this CR? It would set a 
> precedent and give us a clear path towards further thrift union migration.

David and i discussed approaches to this in the first draft, and the 2 sane 
paths forward we found were 1.) patch thrift to make it possible to subclass a 
codegen'd union class, 2.) tackle the problem with our own code generation to 
use different, custom types for database models.  We decided that we would 
benefit from a short-term solution so we can vet the database as a whole, and 
proceed with (2) as it becomes imminent (which it will, soon).


- Bill


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


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-19 Thread Maxim Khutornenko



> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
> We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> 
> I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
> 
>
> 
> David McLaughlin wrote:
> Just to expand on this. The key difference between just storing objects 
> on heap and having some formal storage layer like an RDBMS is that for 
> in-memory, the creation of the underlying entities is performed when I do 
> this:
> 
> 
> LockKey.job(JobsKeys.from(role, env, job).builder());
> 
> 
> For an RDBMS, we have to take this instance and fetch the existing 
> primary key from the table and/or write a new row to the DB. This logic 
> either lives inside the Storage.Mutable implementation, or it leaks out to 
> the application layer where it makes no sense for the existing on heap 
> solutions.
> 
> Maxim Khutornenko wrote:
> | We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> Any thoughts on what that design would look like? I'd rather make an 
> effort to think it through now before locking ourselves into the schema.
> 
> | LockKey.job(JobsKeys.from(role, env, job).builder());
> Understood. However, this kind of specialized syntax is currently used 
> only on the application side (SchedulerThriftInterface) where it makes 
> perfect sense with the store handling it transparently.
> 
> Setting aside this particular issue, my general concern is that we don't 
> seem to know how to handle thrift unions gracefully at this point. There are 
> a few of them in our .thrift files and I am hesitant to move forward without 
> a better story/solution around it.
> 
> David McLaughlin wrote:
> My first take on this is that I'd have a manager/mapper class to 
> represent the LockKey union, which is injected with all the mappers for the 
> underlying types and handles the logic of running the correct underlying 
> create statements based on which field is set in LockKey. It would look 
> identical to the code now, just the type changes. 
>

Any chance we could see it implemented in this CR? It would set a precedent and 
give us a clear path towards further thrift union migration. 


- Maxim


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


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/m

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-19 Thread David McLaughlin



> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
> We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> 
> I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
> 
>
> 
> David McLaughlin wrote:
> Just to expand on this. The key difference between just storing objects 
> on heap and having some formal storage layer like an RDBMS is that for 
> in-memory, the creation of the underlying entities is performed when I do 
> this:
> 
> 
> LockKey.job(JobsKeys.from(role, env, job).builder());
> 
> 
> For an RDBMS, we have to take this instance and fetch the existing 
> primary key from the table and/or write a new row to the DB. This logic 
> either lives inside the Storage.Mutable implementation, or it leaks out to 
> the application layer where it makes no sense for the existing on heap 
> solutions.
> 
> Maxim Khutornenko wrote:
> | We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> Any thoughts on what that design would look like? I'd rather make an 
> effort to think it through now before locking ourselves into the schema.
> 
> | LockKey.job(JobsKeys.from(role, env, job).builder());
> Understood. However, this kind of specialized syntax is currently used 
> only on the application side (SchedulerThriftInterface) where it makes 
> perfect sense with the store handling it transparently.
> 
> Setting aside this particular issue, my general concern is that we don't 
> seem to know how to handle thrift unions gracefully at this point. There are 
> a few of them in our .thrift files and I am hesitant to move forward without 
> a better story/solution around it.

My first take on this is that I'd have a manager/mapper class to represent the 
LockKey union, which is injected with all the mappers for the underlying types 
and handles the logic of running the correct underlying create statements based 
on which field is set in LockKey. It would look identical to the code now, just 
the type changes. 


- David


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


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.j

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-19 Thread Maxim Khutornenko



> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
> We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> 
> I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
> 
>
> 
> David McLaughlin wrote:
> Just to expand on this. The key difference between just storing objects 
> on heap and having some formal storage layer like an RDBMS is that for 
> in-memory, the creation of the underlying entities is performed when I do 
> this:
> 
> 
> LockKey.job(JobsKeys.from(role, env, job).builder());
> 
> 
> For an RDBMS, we have to take this instance and fetch the existing 
> primary key from the table and/or write a new row to the DB. This logic 
> either lives inside the Storage.Mutable implementation, or it leaks out to 
> the application layer where it makes no sense for the existing on heap 
> solutions.

| We would need to add n mappers if the we expand LockKey's fields. Most likely 
we would change the design when that happens. 
Any thoughts on what that design would look like? I'd rather make an effort to 
think it through now before locking ourselves into the schema.

| LockKey.job(JobsKeys.from(role, env, job).builder());
Understood. However, this kind of specialized syntax is currently used only on 
the application side (SchedulerThriftInterface) where it makes perfect sense 
with the store handling it transparently.

Setting aside this particular issue, my general concern is that we don't seem 
to know how to handle thrift unions gracefully at this point. There are a few 
of them in our .thrift files and I am hesitant to move forward without a better 
story/solution around it. 


- Maxim


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


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aur

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-16 Thread David McLaughlin

> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?

We would need to add n mappers if the we expand LockKey's fields. Most likely 
we would change the design when that happens. 

I don't see a way to create a storage abstraction for both in-memory and 
relational databases without seeing artifacts like this in the implementations. 
I think it's also made more complicated by this temporary hybrid state we're 
in. 

> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 50
> > 
> >
> > This is exactly what I was afraid to see. What if the lock key is not 
> > an IJobKey? Would it require a switch on type to determine the right insert 
> > path?

Well, that switch would always need to happen for a union greater than one 
element.

But you are right here and above that I took some shortcuts and went for a 
simpler implementation by relying on the fact that the thrift union is actually 
just one type. I left a comment on this somewhere, but can add more docs if 
that helps. 

> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 77
> > 
> >
> > Can we use LockRow on the insertion path to solve the problem above?

LockRow wouldn't really solve the underlying problem - that none of our Thrift 
types have a field to store the primary keys on the tables, so DbLockStore 
doesn't have all the information it needs to resolve the relationship. So 
*somewhere* you need take a LockRow/ILock/LockKey and ensure it exists in the 
DB before trying to save the lock (see the insert SQL in LockMapper.xml for the 
weirdness we need to do to work around this). 

Ultimately I think it makes a lot of sense to encapsulate this logic and the 
error handling in DbLockStore, rather than push off that responsibility to the 
application layer. 

> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, lines 
> > 10-19
> > 
> >
> > Inline with my above comments, this implementation will not scale when 
> > adding new LockKey types. Is it possible to have a join table between locks 
> > and job_keys to abstract out the dependency? 
> > 
> > Something like: 
> > locks(..., FK:lock_key_id)
> > lock_keys(lock_key_id, lock_type, FK:lock_type_id -> job_key_id)
> > job_keys(job_key_id, ...)

That looks like an unnecessary intermediate table to me? You could have the 
exact same effect by inlining those columns into the locks table. 

- David

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

On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storag

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-16 Thread David McLaughlin



> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > 
> >
> > Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
> We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
> 
> I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
> 
>

Just to expand on this. The key difference between just storing objects on heap 
and having some formal storage layer like an RDBMS is that for in-memory, the 
creation of the underlying entities is performed when I do this:


LockKey.job(JobsKeys.from(role, env, job).builder());


For an RDBMS, we have to take this instance and fetch the existing primary key 
from the table and/or write a new row to the DB. This logic either lives inside 
the Storage.Mutable implementation, or it leaks out to the application layer 
where it makes no sense for the existing on heap solutions. 


- David


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


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-16 Thread Maxim Khutornenko


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



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Breaking the abstraction by having a JobKeyMapper here is quite 
unfortunate. Does it mean we would have to add n mappers here if we add n new 
fields into the LockKey? Any way this could be generalized or abstracted out?



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


This is exactly what I was afraid to see. What if the lock key is not an 
IJobKey? Would it require a switch on type to determine the right insert path?



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Can we use LockRow on the insertion path to solve the problem above?



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


Reword?



src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql


Inline with my above comments, this implementation will not scale when 
adding new LockKey types. Is it possible to have a join table between locks and 
job_keys to abstract out the dependency? 

Something like: 
locks(..., FK:lock_key_id)
lock_keys(lock_key_id, lock_type, FK:lock_type_id -> job_key_id)
job_keys(job_key_id, ...)


- Maxim Khutornenko


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-15 Thread David McLaughlin



> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml, 
> > line 8
> > 
> >
> > Re: style, for some reason the lack of indent here doesn't sit well 
> > with me, i would find this more readable:
> > 
> >   INSERT INTO job_keys (
> > role,
> > environment,
> > name
> >   ) VALUES (
> > #{role},
> > #{environment},
> > #{name}
> >   )
> > 
> > Any other thoughts on this?

+1


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, 
> > line 29
> > 
> >
> > A comment on the importance of an  tag is sure to save someone time 
> > in the future.  You should especially mention that  is a must when the 
> > resultMap only has associations.

Good catch. It's actually _not_ required here.


- David


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


On May 6, 2014, 11 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 6, 2014, 11 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-13 Thread Bill Farner


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



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


please doc



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Prefer 'static final' for constants, and SNAKE_CASE naming.  Name 
convention would be something like ROW_TO_LOCK.

Also, prefer to place constants near where they're used.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


extra newline



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


You can remove @Transactional from all of these, they should reside only in 
DbStorage.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Please either address TODO or leave a more permanent comment explaining the 
thought process for swallowing the exception.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


remove TODO for now.

However, FYI our convention is to always assign TODOs:

  TODO(davmclau): xxx.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Fits on a single line (barely)



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


please doc



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


Can you leave the fully-qualified class name of JdbcHelper to disambiguate? 
 Also, please explain why we need to do this.  (Reminder - it was because 
mybatis' default uses a URL that H2 can't use.)



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


You might even want to inline the magic strings here, with a good comment.  
This would keep the workaround in one place:

  // We would ideally just install the mybatis-provided helper here:
  //   install(JdbcHelper.H2_IN_MEMORY_PRIVATE);
  // Unfortunately, the H2 URL used in that constant is invalid as
  // far as H2 is concerned.  As a workaround, we clone the JdbcHelper
  // behavior here.
  
bindConstant().annotatedWith(named("JDBC.driver")).to(Driver.class.getName());
  bindConstant().annotatedWith(named("JDBC.url")).to("jdbc:h2:mem:");

If you choose to go this route, it obviates a few of the above comments.



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


A comment here would help the drive-by reader not need to drill into 
mybatis for further understanding:

// The environment ID allows SQL templates to expand differently depending 
on the
// environment used (e.g. different databases).  Since we only intend to 
use H2,
// and there is no lock-in, we use an unnamed environment.



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


// Full auto-mapping enables population of nested objects.
// Docs on settings can be found here:
// http://mybatis.github.io/mybatis-3/configuration.html#settings



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


please doc, and be sure to mention that this class is in a migratory phase, 
taking over for MemStorage.



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


When the method signature wraps, our convention is to leave an empty line 
to pad the signature:

  public T longMethodSignature(Args args)
  throws ExceptionType {

// first body line
  }



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


@Transactional



src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java


please doc.  ditto for methods



src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java


doc interface + methods



src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java


More details would be nice (if you copy, please wrap to 100 cols):

  Temporary module to wire the two

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-13 Thread David McLaughlin


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

(Updated May 13, 2014, 6:27 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Changes
---

Removing myself as a reviewer since i paired on this code, and to prevent lazy 
reviewer consensus on my ship it.


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-13 Thread David McLaughlin



> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 54
> > 
> >
> > Formatting nit:
> > 
> >   TODO(davmclau)
> > 
> > as opposed to:
> > 
> >   TODO (davmclau)
> > 
> > Not a big deal, but makes grepping easier

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 59
> > 
> >
> > period at the end to make it clear that your thought wasn't cut off.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 77
> > 
> >
> > This is just as readable if named TO_ROW, and as a bonus the signature 
> > fits on one line.

done.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 96
> > 
> >
> > s/TODO /TODO/

ack. 


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java, 
> > line 29
> > 
> >
> > Please punctuate sentences to make it clear words weren't accidentally 
> > lopped off.
> > 
> > Here and elsewhere in this diff.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java, 
> > line 34
> > 
> >
> > s/  / /

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java, 
> > line 23
> > 
> >
> > s/This class represents the/The/

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java, 
> > line 27
> > 
> >
> > + Note that this must be reworked if other fields are introduced into 
> > LockKey.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml, 
> > line 19
> > 
> >
> > Consider extracting a fragment for the WHERE clause (used 2x).

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, 
> > line 8
> > 
> >
> > +2 indent

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, 
> > line 21
> > 
> >
> > +2 indent

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 63
> > 
> >
> > s/throws RuntimeException //

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 70
> > 
> >
> > MutateWork.Quiet allows you to remove the Exception from your 
> > signature.  Here and below.

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 73
> > 
> >
> > colon should be space-padded:
> > 
> >   for (ILock lock : locks) {
> > 
> > here and below

ack.


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 104
> > 
> >
> > s/final //, applies to ~all test cases

So the rule here is you don't bother applying final to variables that are only 
visible inside the method?


> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 108
> > 
> >
> > this function would help cut down a bunch of redundancy in this class:
> > 
> >   private static ILock makeLock(JobKey job) {
> >

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-13 Thread Bill Farner



> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 104
> > 
> >
> > s/final //, applies to ~all test cases
> 
> David McLaughlin wrote:
> So the rule here is you don't bother applying final to variables that are 
> only visible inside the method?

That's right.  I agree that it's somewhat inconsistent, but useful for brevity.


- Bill


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


On May 13, 2014, 5:49 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 5:49 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-13 Thread David McLaughlin



> On May 12, 2014, 8:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, 
> > line 108
> > 
> >
> > this function would help cut down a bunch of redundancy in this class:
> > 
> >   private static ILock makeLock(JobKey job) {
> > ...
> >   }
> 
> David McLaughlin wrote:
> ack. 
> 
> Since it is useful to be able to differentiate between two locks (and we 
> do this in at least one test), I'm going to add two methods. One which 
> returns a Lock and one which returns an ILock. 
> 
>

Err, scratch that - realised there was no need for the two different tokens on 
those locks. So now just makeLock. 


- David


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


On May 13, 2014, 5:49 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 13, 2014, 5:49 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-13 Thread David McLaughlin


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

(Updated May 13, 2014, 5:49 p.m.)


Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.


Changes
---

Review feedback. 


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-13 Thread David McLaughlin


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

(Updated May 13, 2014, 5:49 p.m.)


Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-12 Thread Bill Farner


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



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


Formatting nit:

  TODO(davmclau)

as opposed to:

  TODO (davmclau)

Not a big deal, but makes grepping easier



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


period at the end to make it clear that your thought wasn't cut off.



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java


This is just as readable if named TO_ROW, and as a bonus the signature fits 
on one line.



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java


s/TODO /TODO/



src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java


Perfect.



src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java


Please punctuate sentences to make it clear words weren't accidentally 
lopped off.

Here and elsewhere in this diff.



src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java


s/  / /



src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java


s/This class represents the/The/



src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java


+ Note that this must be reworked if other fields are introduced into 
LockKey.



src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml


Consider extracting a fragment for the WHERE clause (used 2x).



src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml


+2 indent



src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml


+2 indent



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java


s/throws RuntimeException //



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java


MutateWork.Quiet allows you to remove the Exception from your signature.  
Here and below.



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java


colon should be space-padded:

  for (ILock lock : locks) {

here and below



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java


s/final //, applies to ~all test cases



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java


this function would help cut down a bunch of redundancy in this class:

  private static ILock makeLock(JobKey job) {
...
  }



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java


This points out a leaky abstraction.  It's probably good to adhere to the 
existing expectations that StorageException is thrown.  Simplest place to do 
this is a catch/throw in DbStorage.


- Bill Farner


On May 9, 2014, 11:47 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 9, 2014, 11:47 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-12 Thread Bill Farner



> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 98
> > 
> >
> > When the method signature wraps, our convention is to leave an empty 
> > line to pad the signature:
> > 
> >   public T longMethodSignature(Args args)
> >   throws ExceptionType {
> > 
> > // first body line
> >   }
> 
> David McLaughlin wrote:
> fixed. is it possible to add a rule for this to checkstyle?

Unfortunately this is not supported :-/

http://sourceforge.net/p/checkstyle/feature-requests/279/


- Bill


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


On May 9, 2014, 11:47 p.m., David McLaughlin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> ---
> 
> (Updated May 9, 2014, 11:47 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-335
> https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-12 Thread David McLaughlin



> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 35
> > 
> >
> > please doc

done.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 39
> > 
> >
> > Prefer 'static final' for constants, and SNAKE_CASE naming.  Name 
> > convention would be something like ROW_TO_LOCK.
> > 
> > Also, prefer to place constants near where they're used.

done.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 54
> > 
> >
> > You can remove @Transactional from all of these, they should reside 
> > only in DbStorage.

done.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 46
> > 
> >
> > extra newline

removed.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 60
> > 
> >
> > Please either address TODO or leave a more permanent comment explaining 
> > the thought process for swallowing the exception.

improved comment.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 82
> > 
> >
> > remove TODO for now.
> > 
> > However, FYI our convention is to always assign TODOs:
> > 
> >   TODO(davmclau): xxx.

removed.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 89
> > 
> >
> > Fits on a single line (barely)

fixed.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 39
> > 
> >
> > please doc

done.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 42
> > 
> >
> > Can you leave the fully-qualified class name of JdbcHelper to 
> > disambiguate?  Also, please explain why we need to do this.  (Reminder - it 
> > was because mybatis' default uses a URL that H2 can't use.)

inlined constants as suggested below.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 66
> > 
> >
> > You might even want to inline the magic strings here, with a good 
> > comment.  This would keep the workaround in one place:
> > 
> >   // We would ideally just install the mybatis-provided helper here:
> >   //   install(JdbcHelper.H2_IN_MEMORY_PRIVATE);
> >   // Unfortunately, the H2 URL used in that constant is invalid as
> >   // far as H2 is concerned.  As a workaround, we clone the JdbcHelper
> >   // behavior here.
> >   
> > bindConstant().annotatedWith(named("JDBC.driver")).to(Driver.class.getName());
> >   bindConstant().annotatedWith(named("JDBC.url")).to("jdbc:h2:mem:");
> > 
> > If you choose to go this route, it obviates a few of the above comments.

done.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 75
> > 
> >
> > A comment here would help the drive-by reader not need to drill into 
> > mybatis for further understanding:
> > 
> > // The environment ID allows SQL templates to expand differently 
> > depending on the
> > // environment used (e.g. different databases).  Since we only intend 
> > to use H2,
> > // and there is no lock-in, we use an unnamed environment.

done.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 77
> > 
> >
> > // Full auto-mapping enables population of nested objects.
> > // Docs on settings can be found here:
> > // http://mybatis.github.io/mybatis-3/configuration.html#settings

done.


> On May 7, 2014, 9:01 p.m., Bill Farner wrote:
> > src/main/java/org/apache/au

Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)

2014-05-12 Thread David McLaughlin


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

(Updated May 9, 2014, 11:47 p.m.)


Review request for Aurora, Kevin Sweeney, Maxim Khutornenko, and Bill Farner.


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


Repository: aurora


Description
---

Initial attempt at h2/DB storage implementation (LockStore only)


Diffs (updated)
-

  build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
db1bec4f508c8908f212aa541fb86e041a8c471c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
4b33fe5dd8223ff04060de0fe16b1c0759ead956 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
c851eeb412b17097ff42abce2b7a42fc1c249013 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
4d43f47de75a8bef06f106f563cc071df4cdf093 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
a6319f65bff07db66197e6476647117df6be5c9d 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
283976ab0554dbe6700bb0d2a1b7702c969227e8 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
53923627c827131ee4bd93e5c4865d042aee501b 
  src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
PRE-CREATION 
  src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
34377430268002e8e8e5bc803b409e092bb86720 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
a5191500b2958253e14843089a15a1ffd58e6846 

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


Testing
---


Thanks,

David McLaughlin

43 matches

Mail list logo