----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60942/#review184226 -----------------------------------------------------------
Overall looks good to me. Just a couple of questions about the migration changes. I also think it'd be good for someone at Twitter who's actively working on Aurora to take a look at this to be sure I didn't miss anything. src/main/java/org/apache/aurora/scheduler/storage/db/migration/V004_CreateTaskResourceTable.java Lines 34-41 (original), 33 (patched) <https://reviews.apache.org/r/60942/#comment260316> I wonder if a better option, than having this weird, empty migration, would be to leave the create/drop table in place, but simply remove the migrate script. The issue here was because the migrate script depended on fields that are being removed, it wasn't feasible to leave this migration as-is, right? At the very least we should add comments here explaining what's going on. src/main/java/org/apache/aurora/scheduler/storage/db/migration/V011_DropResourceFields.java Lines 59 (patched) <https://reviews.apache.org/r/60942/#comment260317> It seems strange to me that this migration depends on the task_resource table existing, but we're updating the above migration to never create it. How does this work for someone who might be updating from an early version of Aurora where V0004 was never applied? I know generally we only support +/- 1 version, so we can get away with this if necessary, but I think it's more argument for leaving the add table in the above migration, and just dropping the migrate script? - Joshua Cohen On Aug. 24, 2017, 7:05 p.m., Nicolás Donatucci wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60942/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2017, 7:05 p.m.) > > > Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1707 > https://issues.apache.org/jira/browse/AURORA-1707 > > > Repository: aurora > > > Description > ------- > > Removed task level resource fields from the DB and the thrift-API. > To do this, a new DB migration was added. When upgrading, it just drops the > task level resource fields. When downgrading, it creates the fields again and > populates them with information from the task_resource table. > > IMPORTANT: One of the python client tests is failing (test_cron_diff). This > is not serious, I think it is a problem with the ordering of the elements of > Resources (had similar problems with other python client tests that instead > of comparing the Resources as a set, compared them as lists and thus order > mattered). Nevertheless, I could not fully understand the code of that test. > I was hoping someone could give me a hand with that. > But then again, it is a smaller issue and so the patch can start being > reviewed. > > Issue Related: AURORA-1707 > > > Diffs > ----- > > RELEASE-NOTES.md fd2618fee8ef05091849e177bd99fc321548be90 > api/src/main/thrift/org/apache/aurora/gen/api.thrift > 3749531b5412d7ca217736aa85eed8e6606225ad > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 186fa1b3a4780c0536fb486d50a33133258110cd > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java > d2eb6aa6e4a155b2d28debab2ca10dfc76d57213 > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java > cda55c55680a19ed421299a8949299b21949787b > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V004_CreateTaskResourceTable.java > af106a8a9ee8c14122e98bcc0ec44b616f21d61f > > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V011_DropResourceFields.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java > 138cd5316adc73eed24fc7accc53885dd5d5bee5 > src/main/python/apache/aurora/client/cli/diff_formatter.py > 78717774aa3fbaf83a5fb850bc9f9f4a4038d70f > src/main/python/apache/aurora/client/cli/jobs.py > b79ae56bee0e5692cacf1e66f4a4126b06aaffdc > > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml > 5422183e4a1fe122fc0e1aa871aa75ae102e322d > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > 7a86f47af67adb3e488381d30ddf424549deefbc > src/test/java/org/apache/aurora/scheduler/http/TestUtils.java > 689482c9f6c49bcca781834566edeb975d2f3af2 > > src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java > caaba9b6dff46ff0b037759f1c817a321ae15ee4 > src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java > 43e32eede27bbf26363a3fd1ca34ffe6f8c01a73 > > src/test/java/org/apache/aurora/scheduler/updater/UpdateAgentReserverImplTest.java > 1bc2a778ad3f1543a055023f9ec3fe9e4a9523e3 > src/test/python/apache/aurora/client/cli/test_status.py > b0b7f96d8148f0dd1f6f45d1c5c6f830cabcfd5d > src/test/python/apache/aurora/client/cli/util.py > 43db828ca1cccd91c88f016b4c994fef33182fbf > src/test/python/apache/aurora/config/test_resources.py > 25a20f0b702189744a26b85053db34c37ff5b03c > > > Diff: https://reviews.apache.org/r/60942/diff/8/ > > > Testing > ------- > > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > 1 - Created a new VM with a fresh Aurora 0.18.0, ran the tests so that the db > would have something. Then upgraded to patched version and ran the tests > again. Everything worked and task level resource fields were no longer there. > 2 - Created a new VM with a fresh patched version, ran the tests so that the > db would have something. Then downgraded to 0.18.0 and ran the tests again. > Everything worked and task level resources were there, with the correct > backfilled values. > > > Thanks, > > Nicolás Donatucci > >
