> On April 24, 2016, 4:01 p.m., Bill Farner wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, line 
> > 114
> > <https://reviews.apache.org/r/46459/diff/5/?file=1358264#file1358264line114>
> >
> >     Generalizing all resources to varchar type is quite unfortunate.  Have 
> > you considered using a column per resource type to at least avoid the 
> > string conversion?

Yes, I have considered this approach and ruled it out due to the following:
- To ensure data integrity (e.g. only one column with a non-null value per row) 
we'd have to write a very ugly check constaint on the `task_resource` table 
with a combinatorial explosion in complexity, e.g.:
```
...
CHECK (
     (doubleType IS NOT NULL AND longType IS NULL AND stringType IS NULL)
  OR (longType IS NOT NULL AND doubleType IS NULL AND stringType IS NULL) 
  OR (stringType IS NOT NULL AND doubleType IS NULL AND longType IS NULL)
  ...
);
```
- Cumbersome logic relying on hardcoded `ResourceType` enum values in MyBatis 
to map values to table columns on insert and select OR a non-standard way of 
doing the same in the java layer instead. 
- Schema migrations and mapper/java code changes any time a new java type is 
introduced.

The current approach is much simpler as it does not require ANY changes to the 
SQL layer when a new resource type/kind is introduced. The only code change 
needed is a new `ResourceTypeConverter` interface implementation, which is 
mostly a single liner.


- Maxim


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


On April 23, 2016, 12:16 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46459/
> -----------------------------------------------------------
> 
> (Updated April 23, 2016, 12:16 a.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