-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48559/#review137037
-----------------------------------------------------------




api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 249)
<https://reviews.apache.org/r/48559/#comment202153>

    Please revert. Any fields added after the original structure creation 
should be optional. From https://diwakergupta.github.io/thrift-missing-guide/:
    
    ```
    You should be very careful about marking fields as required. If at some 
point you wish to stop writing or sending a required field, it will be 
problematic to change the field to an optional field?—?old readers will 
consider messages without this field to be incomplete and may reject or drop 
them unintentionally. You should consider writing application-specific custom 
validation routines for your buffers instead. Some have come the conclusion 
that using required does more harm than good; they prefer to use only optional. 
However, this view is not universal.
    ```



src/main/java/org/apache/aurora/scheduler/TierManager.java (line 47)
<https://reviews.apache.org/r/48559/#comment202154>

    s/tasks/task



src/main/java/org/apache/aurora/scheduler/TierManager.java (lines 113 - 115)
<https://reviews.apache.org/r/48559/#comment202155>

    This should be possible to simplify now. The moment scheduler restarts to 
this version there will be no way to _not have_ a tier value set.



src/main/java/org/apache/aurora/scheduler/TierManager.java (lines 124 - 127)
<https://reviews.apache.org/r/48559/#comment202200>

    same here, no need to check isSetTier



src/main/java/org/apache/aurora/scheduler/TierManager.java (lines 131 - 133)
<https://reviews.apache.org/r/48559/#comment202202>

    This can be refactored as well now.



src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java 
(lines 99 - 101)
<https://reviews.apache.org/r/48559/#comment202204>

    There should be an else block here as well to properly set `isProduction` 
in case tier is set. This may become needed when we upgrade to v+1 and then 
rollback to v. Without backfilling the `isProduction` will be set to false for 
everything regardless of the tier. While that's likely not going to cause 
behavior change (i.e. we already user tier internally), it may cause 
consistency problems in the UI (e.g. tier <> isProduction) and/or downstream 
systems consuming our schema.
    
    The same is true about the DB upgrade script above.


- Maxim Khutornenko


On June 10, 2016, 4:52 p.m., Mehrdad Nurolahzade wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48559/
> -----------------------------------------------------------
> 
> (Updated June 10, 2016, 4:52 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> AURORA-1710 Make 'tier' required and remove support for 'production' flag in 
> Job configuration - Backfill portion
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3e6daf444453dd563dd7a2d494cc95e9a0aba0b6 
>   src/main/java/org/apache/aurora/scheduler/TierManager.java 
> af54cab73a80a5120b1a77fd985dfbaf568d786c 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> 3ea0992eb0a9930a4db9eb4b7fcab82689495c1f 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  0e9562020c298e685e6c2efd18933818b03a5000 
>   
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
>  d08873c88f159eb65b582840b48b7ff604862c31 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V006_PopulateTierField.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c981a05e61cb053a05144c702c9ffafeb0af8260 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> 8eed1fc680b0c4fb27d8a353b7f804ae09058156 
>   src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java 
> 0a307fe8d8238c23a526d5c3ee500e1de0761703 
>   src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 
> d4b71f8dbb674384ccbbd9e76f510d127e480e32 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  2e322d217fc9dc75c51b57607a5547745206fb9f 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> e870087e3d47906559410ff76515457f4ff99ff5 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> be1132b439948104458efdc82a6bbee43c20c4fd 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  e0cf602ead1530301b09eff60287b8fa48be63e8 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java 
> 0a2cd3d5b01c389f99fca169227aac35436d474b 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> 4f8158546f3eba8f79d653ad7a30f83d66cbce83 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  ecdc62ae3b21b73b6a6af80bb9855867a7e965e0 
> 
> Diff: https://reviews.apache.org/r/48559/diff/
> 
> 
> Testing
> -------
> 
> Manual under Vagrant:
> - Deployed old scheduler (with tier backfill support), created a job without 
> tier, upgraded scheduler, noticed that tier has been backfilled
> - Tried the above scenario with both -use_beta_db_task_store=true and 
> -use_beta_db_task_store=false configuration flags
> - Verified that if tier is already set it would not be altered
> - Verified that it works both when production = 'true' and production = 
> 'false'
> 
> End to End:
> ```
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> ...
> 
> *** OK (All tests passed) ***
> 
> mesos-master start/running, process 26886
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> real  17m53.514s
> user  0m1.443s
> sys   0m0.624s
> ```
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Reply via email to