aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxim Khutornenko <ma...@apache.org>
Subject Re: Review Request 46459: Schema changes for resource management refactoring
Date Mon, 25 Apr 2016 16:46:54 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message