> On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line > > 35 > > <https://reviews.apache.org/r/21132/diff/1/?file=575743#file575743line35> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575743#file575743line39> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575743#file575743line54> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575743#file575743line46> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575743#file575743line60> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575743#file575743line82> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575743#file575743line89> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575744#file575744line39> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575744#file575744line42> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575744#file575744line66> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575744#file575744line75> > > > > 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 > > <https://reviews.apache.org/r/21132/diff/1/?file=575744#file575744line77> > > > > // 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/aurora/scheduler/storage/db/DbStorage.java, line 41 > > <https://reviews.apache.org/r/21132/diff/1/?file=575745#file575745line41> > > > > please doc, and be sure to mention that this class is in a migratory > > phase, taking over for MemStorage. done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 98 > > <https://reviews.apache.org/r/21132/diff/1/?file=575745#file575745line98> > > > > 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 > > } fixed. is it possible to add a rule for this to checkstyle? > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line > > 113 > > <https://reviews.apache.org/r/21132/diff/1/?file=575745#file575745line113> > > > > @Transactional done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java, > > line 22 > > <https://reviews.apache.org/r/21132/diff/1/?file=575746#file575746line22> > > > > please doc. ditto for methods done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java, line > > 24 > > <https://reviews.apache.org/r/21132/diff/1/?file=575747#file575747line24> > > > > doc interface + methods done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java, > > line 24 > > <https://reviews.apache.org/r/21132/diff/1/?file=575748#file575748line24> > > > > 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. done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java, > > line 33 > > <https://reviews.apache.org/r/21132/diff/1/?file=575748#file575748line33> > > > > checkNotNull, ditto below done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java, > > line 22 > > <https://reviews.apache.org/r/21132/diff/1/?file=575749#file575749line22> > > > > Please doc, especially to record our findings that led us to require > > this wrapper class. done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java, > > line 90 > > <https://reviews.apache.org/r/21132/diff/1/?file=575751#file575751line90> > > > > please revert, if only to make it explicit that this change did not > > need to touch LogStorageModule. reverted. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, > > line 34 > > <https://reviews.apache.org/r/21132/diff/1/?file=575756#file575756line34> > > > > 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? done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml, > > line 41 > > <https://reviews.apache.org/r/21132/diff/1/?file=575756#file575756line41> > > > > s/JOIN /JOIN / fixed. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, > > line 40 > > <https://reviews.apache.org/r/21132/diff/1/?file=575758#file575758line40> > > > > You already know this, but please add more coverage here. done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, > > line 44 > > <https://reviews.apache.org/r/21132/diff/1/?file=575758#file575758line44> > > > > 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. > > done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, > > line 55 > > <https://reviews.apache.org/r/21132/diff/1/?file=575758#file575758line55> > > > > this should probably be removed now, but please add a TODO in DbStorage > > to make sure MyBatis logging is configured properly. done. > On May 7, 2014, 9:01 p.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java, > > line 85 > > <https://reviews.apache.org/r/21132/diff/1/?file=575758#file575758line85> > > > > This is a good time to carve out a helper function to save a lock. done. - David ----------------------------------------------------------- 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 > >
