-----------------------------------------------------------
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
<https://reviews.apache.org/r/21132/#comment76192>

    please doc



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java
<https://reviews.apache.org/r/21132/#comment76194>

    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
<https://reviews.apache.org/r/21132/#comment76195>

    extra newline



src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java
<https://reviews.apache.org/r/21132/#comment76196>

    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
<https://reviews.apache.org/r/21132/#comment76197>

    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
<https://reviews.apache.org/r/21132/#comment76199>

    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
<https://reviews.apache.org/r/21132/#comment76200>

    Fits on a single line (barely)



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java
<https://reviews.apache.org/r/21132/#comment76201>

    please doc



src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java
<https://reviews.apache.org/r/21132/#comment76202>

    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
<https://reviews.apache.org/r/21132/#comment76203>

    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
<https://reviews.apache.org/r/21132/#comment76204>

    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
<https://reviews.apache.org/r/21132/#comment76205>

    // 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
<https://reviews.apache.org/r/21132/#comment76206>

    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
<https://reviews.apache.org/r/21132/#comment76207>

    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
<https://reviews.apache.org/r/21132/#comment76208>

    @Transactional



src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java
<https://reviews.apache.org/r/21132/#comment76209>

    please doc.  ditto for methods



src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java
<https://reviews.apache.org/r/21132/#comment76210>

    doc interface + methods



src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java
<https://reviews.apache.org/r/21132/#comment76211>

    More details would be nice (if you copy, please wrap to 100 cols):
    
      Temporary module to wire the two partial storage implementations together 
as we
      migrate from MemStorage to DbStorage.  This accepts two {@link 
KeyFactory}s,
      one that references the binding scope for the feature-complete 
write-behind
      volatile storage system, and one for the binding scope of the new and
      partially-implemented storage system.
      <p>
      Once the new storage system is feature-complete, this module will be 
deleted
      as the binding bridge is no longer necessary.



src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java
<https://reviews.apache.org/r/21132/#comment76212>

    checkNotNull, ditto below



src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java
<https://reviews.apache.org/r/21132/#comment76213>

    Please doc, especially to record our findings that led us to require this 
wrapper class.



src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java
<https://reviews.apache.org/r/21132/#comment76214>

    please revert, if only to make it explicit that this change did not need to 
touch LogStorageModule.



src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml
<https://reviews.apache.org/r/21132/#comment76215>

    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?



src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml
<https://reviews.apache.org/r/21132/#comment76216>

    A comment on the importance of an <id> tag is sure to save someone time in 
the future.  You should especially mention that <id> is a must when the 
resultMap only has associations.



src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml
<https://reviews.apache.org/r/21132/#comment76218>

    As a person who just skimmed the mybatis docs, i would immediately wonder 
why an association was not used to populate lock.key.  Can you leave a comment 
explaining why that could not be done?



src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml
<https://reviews.apache.org/r/21132/#comment76217>

    s/JOIN  /JOIN /



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java
<https://reviews.apache.org/r/21132/#comment76219>

    You already know this, but please add more coverage here.



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java
<https://reviews.apache.org/r/21132/#comment76220>

    please make this private
    
    Also, you can make this a bit easier for the caller:
    
      private void assertLocks(final ILock... expected) {
        assertEquals(
            ImmutableSet.<ILock>builder().add(expected).build(),
            storage.consistentRead(...)
        );
      }
    
    Then the call sites don't need the ImmutableSet.of() wrappers.
      



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java
<https://reviews.apache.org/r/21132/#comment76221>

    this should probably be removed now, but please add a TODO in DbStorage to 
make sure MyBatis logging is configured properly.



src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java
<https://reviews.apache.org/r/21132/#comment76222>

    This is a good time to carve out a helper function to save a lock.


- Bill Farner


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

Reply via email to