> On April 22, 2016, 3:15 p.m., Joshua Cohen wrote:
> > Overall looks good to me. One question though: what happens in the scenario 
> > where we receive a TaskConfig that has both the individual task-level 
> > resources populated as well as the new resource collection. Further, what 
> > happens in that scenario if the values don't match? I don't think we can 
> > rely on the client to send us valid data because there are lots of users 
> > who generate thrift requests directly, rather than using the client.

Doesn't `ThriftBackfill.backfillTask` already address this problem? If 
`TaskConfig.resources` is not empty the field-level fields are unconditionally 
rewritten to match. I suppose we could throw an error in this case but I think 
it's more robust to ignore deprecated fields and default to the new replacement 
fields instead. I plan to mark all fields as deprecated and add release notes 
when scheduler is fully moved to use `TaskConfig.resources`.


> On April 22, 2016, 3:15 p.m., Joshua Cohen wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml,
> >  line 161
> > <https://reviews.apache.org/r/46459/diff/2/?file=1356072#file1356072line161>
> >
> >     Should this be included as part of the initial query rather than 
> > requring a sub-select?

That's what I started with but eventually had to change it to subselect as join 
did not work well with a case of empty resources (which is still possible to 
hit when scheduler restarts to a new version and data is about to be migrated). 
I presume this is the exact reason why the optional `taskLinks` has also been 
populated via a sub-select.

We can revisit it in the next version when all resources are migrated and old 
fields are being removed.


> On April 22, 2016, 3:15 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/resources/ResourceTypeConverter.java,
> >  lines 49-52
> > <https://reviews.apache.org/r/46459/diff/2/?file=1356057#file1356057line49>
> >
> >     Would it make sense to make this a default method on the interface?

This is an excellent idea. Done.


> On April 22, 2016, 3:15 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java,
> >  line 322
> > <https://reviews.apache.org/r/46459/diff/2/?file=1356054#file1356054line322>
> >
> >     Do you think it would make sense to find all instances of resources 
> > with multiple values rather than stopping at the first? That way if someone 
> > specified multiple invalid resources they can avoid multiple round trips to 
> > correct each instance?
> >     
> >     (I suppose it does potentially open up the ability for a malicious user 
> > to craft a config that could cause the scheduler to perform extra work, but 
> > I'm not sure we're really protected against that scenario in general)

We usually just error out on the first error found instead of trying to 
validate the entire config. In this particular case though I suppose it's easy 
to validate all resources without any extra risk. Changed.


- Maxim


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


On April 21, 2016, 11:03 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46459/
> -----------------------------------------------------------
> 
> (Updated April 21, 2016, 11:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Bill Farner, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This is the first patch towards the generic resource management approach 
> described 
> [here](https://docs.google.com/document/d/1J9SIswRMpVKQpnlvJAMAJtKfPP7ZARFknuyXl-2aZ-M/edit#).
> 
> Apologies for the large diff but this is the minimal logically correct set of 
> changes to ensure proper schema migration/backfilling. 
> 
> A few design/implementation notes:
> - All resources are stored in a single table with values converted to String. 
> This way we can handle additional resource types by just adding a 
> `ResourceTypeConverter` for the new type. Among other benefits the increased 
> human readability of SQL data simplifying ad-hoc analysis
> - The requested ports have been flatmapped together with other resource types 
> to simplify handling
> - Resource details are described in `ResourceType` that provides translation 
> bridge between generic representation and specific resource types
> - `IResource` immutable wrapper got couple of new methods:
>   - `public static Resource newBuilder(int id, Object value)`: for creating 
> mutable resource values in a generic way when reloading resources from DB;
>   - `public Object getRawValue()`: for accessing resource value in a generic 
> way when storing it in DB;
> 
> 
> Notes on data migration/backfilling:
> - The `resource_types` table needs to be populated before any migration 
> happens as it provides foreign keys for the `task_resource`
> - Next, `task_resource` data migration covering task configs already present 
> in the DB (e.g. those inserted by job updates or in case of `DBTaskStore` 
> enabled)
> - `SnapshotStoreImpl` backfilling covers data not present in the dbsnapshot 
> (e.g. `DBTaskStore` disabled)
> - `LogStorage` backfilling covers transaction replays
> - `ConfigurationManager` backfilling covers external RPC handling between 
> v-1/v+1 client/server combinations
> 
> Also, made some test changes to ensure our tests pass with or without 
> `DBTaskStore` enabled.
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 0cf4d6e428806c2b17bd8a824a0bd4fc83c80766 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d91c742c7783905214de563321771ce33dba74a3 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  041cec38f1f96985bbf85468ece0ebbcef2f2486 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/GsonMessageBodyHandler.java
>  44295f80ba8b464d502e72c11c517fac28716c59 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> b6fc949d31371304c2e71363dd61fbf40cfd0ac8 
>   
> src/main/java/org/apache/aurora/scheduler/resources/ResourceTypeConverter.java
>  PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 360914ede6f1978a071a9f7a94d1f328bc252e60 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 25160df1be9539c1ecd4613db5deb99a9606a512 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> e778a39ef61e456ad9d2d2aa6f3e5d69e44bf02c 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V003_CreateResourceTypesTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/migration/V004_CreateTaskResourceTable.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/ResourceTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java
>  ed561c65947d41861d80229ad459392a4bceadf1 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DBResource.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> cdd1060401a38e26cd726ec95e2bb617ccf4708c 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> f586186b61a6025ffccef8cafe480aa6dbf9681c 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 
> b6922e1d5bdd188304359b5646be437ed09ec8d1 
>   src/main/java/org/apache/aurora/scheduler/storage/log/ThriftBackfill.java 
> PRE-CREATION 
>   src/main/python/apache/aurora/config/thrift.py 
> be0cd68674a71bd4baadf276f40a4bc0223ce4be 
>   src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
> 3465fe9ab4a4403270854d20dbc1d20dfc40a80d 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  36df7ed5e23e360f23e6130dca7ae1ce234ee2ce 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> 92a0798d4d97920b786c4713f8779a7a32767652 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> c31069047a9d59d6d635a86f673dd23c2b6868f1 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  1ccfe01111e6c7c18046a8193245e537ae207a13 
>   
> src/test/java/org/apache/aurora/scheduler/resources/ResourceTypeConverterTest.java
>  PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/resources/ResourceTypeTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 
> a33f6f7cbfc1e8262585fdf6c59e14c2afc661e0 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> bf9479d60531324578354c4a15fcfdac040e7ffc 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java
>  ff9c1d08a9a99a69e94d634c895d20fa6c8c2f88 
>   
> src/test/java/org/apache/aurora/scheduler/storage/log/ThriftBackfillTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> fd6a40c8a6f0a16f0914b8025fd6207770bd740e 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  50a1fdbb3dad501bf12fc245e32123a8630dbba5 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  e6e79a0547cafeee80d656dff09d7adb52e4c420 
>   src/test/python/apache/aurora/client/cli/test_inspect.py 
> 452699c08d642e7166903d7797c1650c735266db 
>   src/test/python/apache/aurora/config/test_thrift.py 
> 88292d3c4423c0555088a0adaee3c0e62ed0567e 
> 
> Diff: https://reviews.apache.org/r/46459/diff/
> 
> 
> Testing
> -------
> 
> Manual scheduler upgrades/rollbacks in Vagrant.
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to