> On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java, > > line 15 > > <https://reviews.apache.org/r/24431/diff/1/?file=654329#file654329line15> > > > > s/org.apache.aurora.gen.//
Done. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java, > > line 18 > > <https://reviews.apache.org/r/24431/diff/1/?file=654329#file654329line18> > > > > ditto > > > > s/if/if it/ Done. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml, > > line 20 > > <https://reviews.apache.org/r/24431/diff/1/?file=654336#file654336line20> > > > > -2 indent Done. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml, > > line 21 > > <https://reviews.apache.org/r/24431/diff/1/?file=654337#file654337line21> > > > > After this change, can you go back and update DbLockStore to not catch > > PersistenceException? Good point. Done. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, > > line 63 > > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line63> > > > > notNullColumn is new for us, can you add a comment describing why it is > > used? This is actually the only way to avoid creating an empty collection element out of a NULL row produced by LEFT JOIN when there are no elements on the right side. We are lucky this attribute exists at all: https://issues.apache.org/jira/browse/IBATISNET-206 > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, > > line 34 > > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line34> > > > > If you set columnPrefix="j_", you might be able to let mybatis' POJO > > mapping take care of the individual fields. I _think_ you'll still need to > > specify <id> though. If this works here, the approach might apply to other > > resultMaps in here as well. Good suggestion. Refactored a bit to make this happen. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, > > line 65 > > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line65> > > > > columnPrefix on the <collection> should make fields auto-amp, with the > > exception of those that need special handling (id, typeHandler). Done. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, > > line 68 > > <https://reviews.apache.org/r/24431/diff/1/?file=654338#file654338line68> > > > > Ditto re: columnPrefix Done. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml, > > line 59 > > <https://reviews.apache.org/r/24431/diff/1/?file=654340#file654340line59> > > > > Should we start leaning on the database and let 'ON DELETE CASCADE' > > handle child tables appropriately? I realize now i did not do this in > > AttributeMapper.xml, but i think it's worth considering. I tried and failed with that. Here is a related complaint: http://stackoverflow.com/questions/4151316/unable-to-cascade-delete-a-onetoone-member Will stick with current solution until I figure out how to make it work. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, line > > 97 > > <https://reviews.apache.org/r/24431/diff/1/?file=654341#file654341line97> > > > > i think you can s/BIGINT //, AFAICT h2 uses a long regardless. Not a > > bad idea to do this without. Dropped. Now, I wonder if "INT IDENTITY" is actually meaningless. We should probably drop it everywhere. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, > > line 46 > > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line46> > > > > A test case for two updates with the full slew of child entries would > > be nice, to prove that they don't cross wires (lesson learned from my > > recent JOIN flub). Added. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, > > line 85 > > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line85> > > > > please check that these objects are valid, not just present (equality > > rather than size). > > > > this implicitly tests the ordering, so you can skip the checks below. Refactored. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, > > line 135 > > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line135> > > > > You could do this in an @After as a catch-all for every test case. Added. > On Aug. 7, 2014, 3:41 a.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, > > line 136 > > <https://reviews.apache.org/r/24431/diff/1/?file=654342#file654342line136> > > > > In general, favor: > > > > assertEquals(Optional.<Foo>absent(), actual); > > > > over: > > > > assertFalse(actual.isPresent()); > > > > The difference is subtle, but the former produces test output that > > shows the incorrect value, and will point more directly towards the problem. Changed. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24431/#review49860 ----------------------------------------------------------- On Aug. 7, 2014, 12:01 a.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24431/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2014, 12:01 a.m.) > > > Review request for Aurora, David McLaughlin and Bill Farner. > > > Bugs: AURORA-612 > https://issues.apache.org/jira/browse/AURORA-612 > > > Repository: aurora > > > Description > ------- > > Adding SQL support for saving updates and fetching update details. > > The configuration and updateOnlyTheseInstances option support will come > separately. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java > 869590ed3c43c64936af42e60d82f5a6938475d3 > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java > 5d6521813e40eed5cf7e6e0285a7400624c4c5b2 > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java > 8a573f7b04ecc37a19e3886f334f626af577839a > > src/main/java/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java > 844714c08cdaf280c8862e933a95ab009bd9e464 > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/db/LockKeyMapper.java > 618d5b72fd3078ef59571b5f2e33ecce3eb7386c > > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateActionTypeHandler.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/JobUpdateStatusTypeHandler.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java > f014123348e2b229b2bb9be0dce424327ec0dfbe > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobInstanceUpdateEventMapper.xml > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml > 3997f9833a7289a49e4f4972efc3f953674bd41b > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml > PRE-CREATION > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateEventMapper.xml > PRE-CREATION > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateMapper.xml > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > eb2216ae22ae95370d0b5cd15dbb3d61fca79272 > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/24431/diff/ > > > Testing > ------- > > gradle -Pq build > > > Thanks, > > Maxim Khutornenko > >
