Re: Review Request 43262: Backfilling JobConfiguration.Identity
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118089 --- Ship it! Ship It! - Bill Farner On Feb. 5, 2016, 1:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 1:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118088 --- Ship it! Just relaying my homework: The last Identity use is in TaskQuery, but those are never stored, so we're good. - John Sirois On Feb. 5, 2016, 2:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 2:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
> On Feb. 5, 2016, 1:08 p.m., John Sirois wrote: > > Can you note the technique you used for finding all last vestigaes of > > missing backfill in the testing done? > > Was this grep-provable as the last bit to fix for example? > > Maxim Khutornenko wrote: > That was mostly an oversight as we should have backfilled everything > touched in api.thrift: https://reviews.apache.org/r/42811/diff/3#1 > > Bill Farner wrote: > John - do you think this is something we should consider tackling with > codegen? For example, if we could make the `IIdentity` and `ITaskConfig` > code do this it seems like we wouldn't be playing whack-a-mole. > > John Sirois wrote: > I'm not sure what it would look like in codegen, though I'm not confident > enough in where the wrappers are used to be sure there still won't be a raw > unwrapped thrift struct used in a set somewhere. > I do think exploring wrapping the boundary interfaces - thrift rpc > ifaces, log storage, mappers - with an interceptor that has a map of type -> > backfill op though is worth exploring. This is a handwave though since the > map would need to have a key for every thrift object that can contain a > thrift object needing backfill. IE, X needs backfill, but we might have > `A.getB().getX()` so we'd need to backfill A's, B's and X's to get the X's. > > I assume we both agree that the better-than-whac-a-mole solution, if it > exists, can come in 0.13.0 though. > I assume we both agree that the better-than-whac-a-mole solution, if it > exists, can come in 0.13.0 though. Absolutely. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118076 --- On Feb. 5, 2016, 1:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 1:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
> On Feb. 5, 2016, 2:08 p.m., John Sirois wrote: > > Can you note the technique you used for finding all last vestigaes of > > missing backfill in the testing done? > > Was this grep-provable as the last bit to fix for example? > > Maxim Khutornenko wrote: > That was mostly an oversight as we should have backfilled everything > touched in api.thrift: https://reviews.apache.org/r/42811/diff/3#1 > > Bill Farner wrote: > John - do you think this is something we should consider tackling with > codegen? For example, if we could make the `IIdentity` and `ITaskConfig` > code do this it seems like we wouldn't be playing whack-a-mole. > > John Sirois wrote: > I'm not sure what it would look like in codegen, though I'm not confident > enough in where the wrappers are used to be sure there still won't be a raw > unwrapped thrift struct used in a set somewhere. > I do think exploring wrapping the boundary interfaces - thrift rpc > ifaces, log storage, mappers - with an interceptor that has a map of type -> > backfill op though is worth exploring. This is a handwave though since the > map would need to have a key for every thrift object that can contain a > thrift object needing backfill. IE, X needs backfill, but we might have > `A.getB().getX()` so we'd need to backfill A's, B's and X's to get the X's. > > I assume we both agree that the better-than-whac-a-mole solution, if it > exists, can come in 0.13.0 though. > > Bill Farner wrote: > > I assume we both agree that the better-than-whac-a-mole solution, if it > exists, can come in 0.13.0 though. > > Absolutely. Here's what might be really useful in a make-believe world where my annotation work exists in Apache Thrift - annotating structs with key fields such that the generated objects are not value-equals, but keyed off a ubset of fields that form the logical key. That I think would help alot with migrations. - John --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118076 --- On Feb. 5, 2016, 2:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 2:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118086 --- Ship it! Master (8faf499) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing "@ReviewBot retry" - Aurora ReviewBot On Feb. 5, 2016, 9:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 9:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
> On Feb. 5, 2016, 9:48 p.m., John Sirois wrote: > > Just relaying my homework: The last Identity use is in TaskQuery, but those > > are never stored, so we're good. Yeah, that will be dropped when I un-revert https://reviews.apache.org/r/43080 - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118088 --- On Feb. 5, 2016, 9:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 9:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
> On Feb. 5, 2016, 9:08 p.m., John Sirois wrote: > > Can you note the technique you used for finding all last vestigaes of > > missing backfill in the testing done? > > Was this grep-provable as the last bit to fix for example? That was mostly an oversight as we should have backfilled everything touched in api.thrift: https://reviews.apache.org/r/42811/diff/3#1 - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118076 --- On Feb. 5, 2016, 9:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 9:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
> On Feb. 5, 2016, 1:08 p.m., John Sirois wrote: > > Can you note the technique you used for finding all last vestigaes of > > missing backfill in the testing done? > > Was this grep-provable as the last bit to fix for example? > > Maxim Khutornenko wrote: > That was mostly an oversight as we should have backfilled everything > touched in api.thrift: https://reviews.apache.org/r/42811/diff/3#1 John - do you think this is something we should consider tackling with codegen? For example, if we could make the `IIdentity` and `ITaskConfig` code do this it seems like we wouldn't be playing whack-a-mole. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118076 --- On Feb. 5, 2016, 1:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 1:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
Re: Review Request 43262: Backfilling JobConfiguration.Identity
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43262/#review118076 --- Can you note the technique you used for finding all last vestigaes of missing backfill in the testing done? Was this grep-provable as the last bit to fix for example? - John Sirois On Feb. 5, 2016, 2:03 p.m., Maxim Khutornenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43262/ > --- > > (Updated Feb. 5, 2016, 2:03 p.m.) > > > Review request for Aurora, John Sirois and Bill Farner. > > > Bugs: AURORA-1610 > https://issues.apache.org/jira/browse/AURORA-1610 > > > Repository: aurora > > > Description > --- > > Backfilling JobConfiguration.Identity > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java > 9c88f5763d77ec0a304fdb2530ef6b6c1b0f230e > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 13fcf9fd3afc14cf572d4bc7de355ff698b2c222 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > 3b176db12369bc5a2a63cb0018ffca7ef8b9055d > > src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java > 506d5919b9db14e04b10190a5f2d2aef123ac1dc > > Diff: https://reviews.apache.org/r/43262/diff/ > > > Testing > --- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
