> On June 15, 2016, 5:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/TierManager.java, lines 125-127 > > <https://reviews.apache.org/r/48559/diff/3/?file=1418764#file1418764line125> > > > > Reiterating my previous comment: can this (and other places checking > > for tier presence) be simplified now that the tier is a required field? > > Mehrdad Nurolahzade wrote: > This logic is used by ```ThriftBackfill``` to set ```tier``` when it is > not already set. > Shouldn't this be removed in the next release when we drop the back fill > logic?
Do you expect to use `getTierName()` anywhere else besides `ThriftBackfill`? If not this logic should be moved to its only consumer. > On June 15, 2016, 5:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java, line 14 > > <https://reviews.apache.org/r/48559/diff/3/?file=1418765#file1418765line14> > > > > Not sure why this class has been moved but if you want to keep it > > please carve it out into a separate patch. Combining functional changes > > with moving files around inflates the diff size and makes it harder to > > understand the commit in retrospect. > > Mehrdad Nurolahzade wrote: > I can revert this package move refactoring. This was required by the > refactorings to ```TaskTestUtil```. > I can alternatively make the constructor of ```TierManagerImpl``` public > (instead of moving them both to the same package). I don't have a strong preference either way. It's acceptable to make constructor public if marked @VisibleForTesting. > On June 15, 2016, 5:41 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V006_PopulateTierField.java, > > lines 50-52 > > <https://reviews.apache.org/r/48559/diff/3/?file=1418768#file1418768line50> > > > > This should be unnecessary as zero is the default value for > > 'production'. > > Mehrdad Nurolahzade wrote: > Isn't this required to resolve the inconsistencies between the choice of > ```production``` and ```tier``` in already scheduled jobs? > For example, if tier has been set to ```revocable``` and production to > ```true``` in a job submitted to the old scheduler, shouldn't we resolve this > by resetting ```production``` to ```false```? > > Mehrdad Nurolahzade wrote: > Actually, thinking twice about this, do we even need a downgrade script? > I mean, we are already modifying ```production``` based on choice of > ```tier``` for scheduled jobs, so it should not be a concern when downgrading > scheduler, right? This is a matter of consistency with `ThriftBackfill` and while not breaking functionality, could be confusing to have something like 'preferred & prod=false' after a rollback. The 'revocable & prod=true' was the invalid combination in the first place, so having rollback fixing consistency here feels right. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48559/#review137763 ----------------------------------------------------------- On June 14, 2016, 5:12 p.m., Mehrdad Nurolahzade wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48559/ > ----------------------------------------------------------- > > (Updated June 14, 2016, 5:12 p.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Repository: aurora > > > Description > ------- > > AURORA-1710 Make 'tier' required and remove support for 'production' flag in > Job configuration - Backfill portion > > > Diffs > ----- > > src/jmh/java/org/apache/aurora/benchmark/JobUpdates.java > f4f8d0037751c9c2096747264c19f6292461b308 > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java > 65f5edce74077f52e98f110fcd17b2f12d673f81 > src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java > 6fd9ee21cf8e0e42e73a68cdf2d231d581278aae > src/jmh/java/org/apache/aurora/benchmark/Tasks.java > e548a09d94b1c6d550f9beec2b7120b64d576f20 > src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java > 7497fb96a578cff64b6484c587e29ec464969e84 > 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/codec/ThriftBinaryCodecTest.java > 999ef064737c3d8a3d7610b40c13736f51742edd > src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java > 05cd78f4c7c7d8dd6eeb6f2f9a3e8f7a167f274d > src/test/java/org/apache/aurora/scheduler/TierManagerTest.java > d4b71f8dbb674384ccbbd9e76f510d127e480e32 > src/test/java/org/apache/aurora/scheduler/TierModuleTest.java > 58d95dcdf31bc920ca1f8822baccc6c37b66e739 > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > 8c8c793813e84bf7ef741f9b6b4ae1e759be1b82 > src/test/java/org/apache/aurora/scheduler/base/JobsTest.java > 13f656f241a8a9a3d339f4053f165070c2669ef3 > src/test/java/org/apache/aurora/scheduler/base/TasksTest.java > 935622bef38cc3d399e8b5b1db84fa21c79c78e1 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 2e322d217fc9dc75c51b57607a5547745206fb9f > > src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java > 5c64ff2994e200b3453603ac5470e8e152cebc55 > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronIT.java > 1c0a3fa84874d7bc185b78f13d2664cb4d8dd72f > > src/test/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImplTest.java > 81440f5689f9538a4c7a9e6700bf03ca89c4ba85 > src/test/java/org/apache/aurora/scheduler/cron/quartz/QuartzTestUtil.java > 3c5ecd698557cafdf8eeacdc472589a379018896 > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > 488eefd14c3e67a41a75c809397c8d19f83cc08a > src/test/java/org/apache/aurora/scheduler/http/MaintenanceTest.java > f94b58b77b7c6ce824914af7e1147e73ad5a7eed > src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java > ccef4ab930a7b7e1c10d611f0852aa65de82e726 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java > 58785bfa37ff214f26e9f94d836e6df40e411c3b > > src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java > 7eb1714d14581a6ab25e85d36a1f3e973380c536 > > src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java > 99c27e8012f10a67ce5f1b84d258e7a5608995c7 > src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java > 2e97a3361feaed71e4f39cbd27cf5afb7d919e31 > > src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java > b9317dc20456f90057ec2bf4d10619a5ae986187 > src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java > 2777d7276d55cc0fe75e7470d0dc8182796d67f4 > src/test/java/org/apache/aurora/scheduler/resources/PortMapperTest.java > 244c9de966311789e97a5fb1a5ab6625c232c7ac > > src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java > 14ac54713acf69fc40807b8cf7345b7a043e1ad9 > > src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java > 2d34729e8f371703ba250b8e82d82c8a2504fcac > > src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java > fba427bd327e7f63b640c8b8753bfdeec3ee31e7 > src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java > 78f440f7546de9ed6842cb51db02b3bddc9a74ff > > src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java > 94f5ca565476f62d72879837a0e7dafabcf30432 > src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java > fbcc4003f7647580b859f4dc08a16c3e471a8f18 > src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java > b4d27f69ad5d4cce03da9f04424dc35d30e8af29 > src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java > c5193238db5cb65373f7cb69e6b38b2e3b051dae > src/test/java/org/apache/aurora/scheduler/stats/ResourceCounterTest.java > 19c1f8e949e31a2338a5cbbbf15228230377a9f5 > > src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java > c316e497a34a45c7ada2ca83a1115e826c0f572f > > src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java > b1593f682f48ea66339bc2372de3e4f14e40be32 > src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java > e870087e3d47906559410ff76515457f4ff99ff5 > > src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java > f47f4a8a492fb43bacd909dc520256ed028531dd > > src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java > 08530397ff75081bde6f07f9d53317b5486e0da4 > > src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java > 3e5296e40ba63dc06a4720f1ff2c1ff046613ea2 > src/test/java/org/apache/aurora/scheduler/storage/log/LogManagerTest.java > 73440517cddda643c0b84cc04cb8463cdea2da28 > 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/storage/log/WriteAheadStorageTest.java > 13726cc11ab09cd4995233d9d31811b97b065275 > > src/test/java/org/apache/aurora/scheduler/storage/mem/StorageTransactionTest.java > 25f34e2bc26c6d4754c1591fad7f2165dd465d32 > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > 4f8158546f3eba8f79d653ad7a30f83d66cbce83 > > src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java > 6f355d6e66c05651fa9b13356dd81b45bee52adc > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > ecdc62ae3b21b73b6a6af80bb9855867a7e965e0 > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > a54d169caebfc211035386f64169ecd983e378d9 > src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java > 56c94b5caf414861212f673a27b84d46c07332e6 > > src/test/java/org/apache/aurora/scheduler/updater/JobUpdateEventSubscriberTest.java > 36f2c657c05a87e78a11a1b0be5779dfd6511ee5 > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java > e157c0dfde5efc418448e138aa008ade742fe816 > src/test/java/org/apache/aurora/scheduler/updater/KillTaskTest.java > e5935f65924e7d9a2491cac8f4c1f575ec657776 > > 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 > >