You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2016/02/04 03:14:29 UTC
Review Request 43172: Add deprecated field storage backfill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/
-----------------------------------------------------------
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
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117751
-----------------------------------------------------------
Ship it!
Master (52c19cf) 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. 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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by John Sirois <js...@apache.org>.
> On Feb. 3, 2016, 7:39 p.m., John Sirois wrote:
> > This looks about right to me.
> >
> > In the general case though for thrift field moves (I was thinking about this problem last night), it seems to me there would need to be a bi-directional fill in-place when the new field locations were introduced to both copy the old field location value (when non-null) to the new location for old releases and vice-versa (the case you have covered here) to allow for rollback from a release that removes the old field locations.
> >
> > IIUC the forward-fill missing here occurred long-ago though, so we're ok.
> >
> > If that makes sense is that about right?
... and furthermore, the bi-drectional fill pattern is needed in-general to cope with the fact that thrift objects are values equals, so any field move requires bi-directional fill to be safe for sets and maps and the like.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117750
-----------------------------------------------------------
On Feb. 3, 2016, 7:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 7:14 p.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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 4, 2016, 2:39 a.m., John Sirois wrote:
> > This looks about right to me.
> >
> > In the general case though for thrift field moves (I was thinking about this problem last night), it seems to me there would need to be a bi-directional fill in-place when the new field locations were introduced to both copy the old field location value (when non-null) to the new location for old releases and vice-versa (the case you have covered here) to allow for rollback from a release that removes the old field locations.
> >
> > IIUC the forward-fill missing here occurred long-ago though, so we're ok.
> >
> > If that makes sense is that about right?
>
> John Sirois wrote:
> ... and furthermore, the bi-drectional fill pattern is needed in-general to cope with the fact that thrift objects are values equals, so any field move requires bi-directional fill to be safe for sets and maps and the like.
We have switched to JobKeys in 0.6.0: https://github.com/apache/aurora/blob/rel/0.6.0-incubating/src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java#L125-L128
Now that we are fully relying on TaskConfig.job, I don't think forward filling is needed any longer. There is simply no way to have a TaskConfig without a job field populated. The opposite is not true as we learned in AURORA-1603.
In general though, I agree that every field deprecation should be followed by a bi-directional backfill to set up forward (introduction of new fields) and backward (removal of deprecated fields) deploys.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117750
-----------------------------------------------------------
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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117750
-----------------------------------------------------------
This looks about right to me.
In the general case though for thrift field moves (I was thinking about this problem last night), it seems to me there would need to be a bi-directional fill in-place when the new field locations were introduced to both copy the old field location value (when non-null) to the new location for old releases and vice-versa (the case you have covered here) to allow for rollback from a release that removes the old field locations.
IIUC the forward-fill missing here occurred long-ago though, so we're ok.
If that makes sense is that about right?
- John Sirois
On Feb. 3, 2016, 7:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 7:14 p.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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Bill Farner <wf...@apache.org>.
> On Feb. 4, 2016, 6:21 a.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.
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.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117808
-----------------------------------------------------------
On Feb. 3, 2016, 6:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 6:14 p.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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
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?
- 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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Bill Farner <wf...@apache.org>.
> On Feb. 4, 2016, 6:21 a.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?
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.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117808
-----------------------------------------------------------
On Feb. 3, 2016, 6:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 6:14 p.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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117808
-----------------------------------------------------------
Fix it, then Ship it!
src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java (line 45)
<https://reviews.apache.org/r/43172/#comment179074>
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.
src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java (line 50)
<https://reviews.apache.org/r/43172/#comment179072>
Finish this thought - there is no actual testing done here.
- John Sirois
On Feb. 3, 2016, 7:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 7:14 p.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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117839
-----------------------------------------------------------
Ship it!
Ship It!
- John Sirois
On Feb. 4, 2016, 10:13 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, 10:13 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/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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117875
-----------------------------------------------------------
Ship it!
Master (42bff19) 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. 4, 2016, 5:13 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 5:13 p.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/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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117902
-----------------------------------------------------------
Cherry-picked reverted patches and ran upgrade/rollback cycle twice in vagrant. Validated with e2e tests and manually.
- Maxim Khutornenko
On Feb. 4, 2016, 8:58 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 8:58 p.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/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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117886
-----------------------------------------------------------
Ship it!
Master (42bff19) 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. 4, 2016, 8:58 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2016, 8:58 p.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/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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/
-----------------------------------------------------------
(Updated Feb. 4, 2016, 8:58 p.m.)
Review request for Aurora, John Sirois and Bill Farner.
Changes
-------
Realized we also need to backfill all tasks and cron jobs.
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 (updated)
-----
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/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
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/
-----------------------------------------------------------
(Updated Feb. 4, 2016, 5:13 p.m.)
Review request for Aurora, John Sirois and Bill Farner.
Changes
-------
Review feedback.
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 (updated)
-----
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/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
Re: Review Request 43172: Add deprecated field storage backfill
Posted by John Sirois <js...@apache.org>.
> On Feb. 4, 2016, 3:37 a.m., Stephan Erb wrote:
> > During this whole odyssee, have we learned a thing which is not reflected here? https://github.com/apache/aurora/blob/master/docs/thrift-deprecation.md
> >
> > If yes, please write it down :-)
I concur, but I'm fine with the doc update coming after this. I'd like to block RC on production bugs and bugs in docs facing users (storage recovery docs for example), but developer docs can slide a bit imo.
I think that doc places a wrong emphasis on dual reads - I think in fact this is bad advice. The key is dual writes (bi-directional filling), at the 1st encounter of any thrift struct. This is the only way to ensure sets and maps will work as expected throughout the application and it frees the rest of the app to read the field from the current single canonical location, whatever that is.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117791
-----------------------------------------------------------
On Feb. 3, 2016, 7:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 7:14 p.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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 4, 2016, 10:37 a.m., Stephan Erb wrote:
> > During this whole odyssee, have we learned a thing which is not reflected here? https://github.com/apache/aurora/blob/master/docs/thrift-deprecation.md
> >
> > If yes, please write it down :-)
>
> John Sirois wrote:
> I concur, but I'm fine with the doc update coming after this. I'd like to block RC on production bugs and bugs in docs facing users (storage recovery docs for example), but developer docs can slide a bit imo.
>
> I think that doc places a wrong emphasis on dual reads - I think in fact this is bad advice. The key is dual writes (bi-directional filling), at the 1st encounter of any thrift struct. This is the only way to ensure sets and maps will work as expected throughout the application and it frees the rest of the app to read the field from the current single canonical location, whatever that is.
I have updated AURORA-1605 to also revisit thrift deprecation instructions.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117791
-----------------------------------------------------------
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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Stephan Erb <st...@dev.static-void.de>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117791
-----------------------------------------------------------
During this whole odyssee, have we learned a thing which is not reflected here? https://github.com/apache/aurora/blob/master/docs/thrift-deprecation.md
If yes, please write it down :-)
- Stephan Erb
On Feb. 4, 2016, 3: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, 3: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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Feb. 4, 2016, 4:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java, line 47
> > <https://reviews.apache.org/r/43172/diff/1/?file=1232009#file1232009line47>
> >
> > Please favor immutable inputs and outputs; return `IJobUpdate`.
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117831
-----------------------------------------------------------
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
>
>
Re: Review Request 43172: Add deprecated field storage backfill
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43172/#review117831
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java (line 47)
<https://reviews.apache.org/r/43172/#comment179093>
Please favor immutable inputs and outputs; return `IJobUpdate`.
- Bill Farner
On Feb. 3, 2016, 6:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43172/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 6:14 p.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
>
>