aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Farner <wfar...@apache.org>
Subject Re: Review Request 43172: Add deprecated field storage backfill
Date Thu, 04 Feb 2016 16:45:14 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message