> On Feb. 4, 2016, 2:21 p.m., John Sirois wrote: > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java, > > line 50 > > <https://reviews.apache.org/r/43172/diff/1/?file=1232013#file1232013line50> > > > > Finish this thought - there is no actual testing done here.
Good catch! Done. > On Feb. 4, 2016, 2:21 p.m., John Sirois wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java, > > line 45 > > <https://reviews.apache.org/r/43172/diff/1/?file=1232006#file1232006line45> > > > > Having a backfill in the db layer strikes me as a red flag; ie all > > thrift inputs to the scheduler should be sanitized as close as possible to > > the input site. I think this means asap in a thrift RPC and when reading > > from the distributed log. > > > > This may be impractical to implement today or else this just may be > > more expedient to getting 0.12.0 out the door with confidence in which case > > I'm fine with it, but I'd like to file a follow-up issue to investigate a > > generic intercepting layer for the thrift RPC interface and the LogStorage > > that could look up thrift structs types in a map and apply a sanitization > > UnaryOperator if found... mod some handwaving. > > Bill Farner wrote: > I agree with the above. I also want to point out (and suggest a comment) > that it is imperative that the backfill routine is logically equivalent to > the way a `TaskConfig` is altered by a storage round-trip. If at any point > that does not hold true, we can encounter the same symptom (duplicate task > configs) again. > > Maxim Khutornenko wrote: > This particular backfill was, in fact, redundant given that we also do it > at the recovery level. I am going to remove it entirely. > > Bill, given the above, how should I interpret your comment now? Are you > suggesting remodeling the schema to what we discussed earlier to explicitly > map deprecated fields to DB columns? > > Bill Farner wrote: > Perhaps just a cautionary comment here is sufficient for now. Given that > we are assuming an invariant with `Maps.uniqueIndex`, an object inserted into > the DB must be `equals()` the same object when retrieved. Done. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43172/#review117808 ----------------------------------------------------------- On Feb. 4, 2016, 2:14 a.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43172/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2016, 2:14 a.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1603 > https://issues.apache.org/jira/browse/AURORA-1603 > > > Repository: aurora > > > Description > ------- > > This change ensures we can un-revert https://reviews.apache.org/r/43104/ when > 0.12.0 ships and still have a graceful rollback from 0.13.0 to 0.12.0. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java > 27f1f33ffc782b3d2a2a9add494c04911659e217 > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java > 042f71dd8bc81d03f2759ee7f7f4b65a098d11e2 > > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java > db90150b48c5b134dde6c69f70ebca82bfdd0c12 > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java > 54defc256ad8a261c6b56ee06ad7fdd16a26b057 > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 7382eca281eeab17d407ed140f16d6a633d8ad72 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 806f50d57e44261e3a1988a3c9bb742442badde8 > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/43172/diff/ > > > Testing > ------- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
