----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67077/#review203818 -----------------------------------------------------------
Fix it, then Ship it! Thanks! Looks good to me overall. I raise a concern about the place of the validation logic below. I don't feel super strongly about this, so I leave this decision up to you. api/src/main/thrift/org/apache/aurora/gen/api.thrift Lines 290-298 (original) <https://reviews.apache.org/r/67077/#comment286135> Please add a changelog entry that explains which fields were removed from the public API. src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java Lines 171-172 (original), 172-176 (patched) <https://reviews.apache.org/r/67077/#comment286131> The previous check ensured _all_ resources were non-negative. The new check only ensures that no negative value is in the list. It could however be the case that one is missing entirely. Thinking about it, this is probably prevented by the validation in the ThriftBackfill class. src/main/java/org/apache/aurora/scheduler/storage/durability/ThriftBackfill.java Lines 127-152 (original), 114-127 (patched) <https://reviews.apache.org/r/67077/#comment286133> Technically we are no longer doing a backfill of deprecated fields. Ideally we would move the validation to somwhere else now (e.g. `QuotaManagerImpl.saveQuota`). I am happy about the refactoring overall. It is therefore fine with me to let this one slide. - Stephan Erb On May 11, 2018, 5:18 a.m., Jing Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67077/ > ----------------------------------------------------------- > > (Updated May 11, 2018, 5:18 a.m.) > > > Review request for Aurora, Renan DelValle and Stephan Erb. > > > Bugs: AURORA-1975 > https://issues.apache.org/jira/browse/AURORA-1975 > > > Repository: aurora > > > Description > ------- > > Remove resource properties from ResourceAggregate > > > Diffs > ----- > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > ef754e32172e7490a47a13e7b526f243ffa3efeb > src/main/java/org/apache/aurora/scheduler/http/Quotas.java > aa68c446af7cf52f37cb72d0f115b1be39457988 > src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java > e2750d7343a1ed83feb9f96014e9e61dc6dda1f0 > > src/main/java/org/apache/aurora/scheduler/storage/durability/ThriftBackfill.java > 4425d025b92425b19eead30ceac0bc2466bc606a > src/main/python/apache/aurora/client/api/__init__.py > f6fd1dd6d7c2bdd5bca3037f501b36badab78c75 > src/main/python/apache/aurora/config/resource.py > b2ebd399ccf069b9914ac4bc624685b2cc3679b0 > src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java > 567586fcfb4dda58dc6308fe3d7fe2b7b433db98 > > src/test/java/org/apache/aurora/scheduler/storage/durability/DurableStorageTest.java > 3dd9ce4039b223cb6156462d089f7062a1cde772 > > src/test/java/org/apache/aurora/scheduler/storage/durability/ThriftBackfillTest.java > 219576baf331c44535a4e2f95fde5c906bdabe4f > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > 2cf66d8154ad3795989ee9026e45af1be509f244 > src/test/python/apache/aurora/admin/test_admin.py > ebe89b504be57a4258eeea7e3edd053daaa30ce9 > src/test/python/apache/aurora/client/cli/test_quota.py > b56629671035ce6bf1c3bf45c9adeb2c4f108ee8 > src/test/python/apache/aurora/config/test_resources.py > 3ac54909991a123b73e00c59d7269431036cda06 > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/current/saveQuota > 6b0d8003a23b9a0e9de78eed42aed14c6b7dd492 > > > Diff: https://reviews.apache.org/r/67077/diff/2/ > > > Testing > ------- > > ./pants test src/test/python/apache/aurora/config > ./pants test src/test/python/apache/aurora/client/cli:cli > ./build-support/jenkins/build.sh > > > Thanks, > > Jing Chen > >
