> On April 22, 2016, 6:55 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/GsonMessageBodyHandler.java,
> >  line 210
> > <https://reviews.apache.org/r/46459/diff/3/?file=1357528#file1357528line210>
> >
> >     Instead of having a switch statement here, perhaps it would be cleaner 
> > if we hace a private static ImmutableMap which mapped the thrift to json 
> > types.

I don't find this indirection any easier to follow. It's also consistent with 
the internal thrift type handling.


> On April 22, 2016, 6:55 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java, line 
> > 143
> > <https://reviews.apache.org/r/46459/diff/3/?file=1357529#file1357529line143>
> >
> >     To prevent human errors when adding more resouces to this enum perhaps 
> > we should make the type here `Resources$_Fields` and do the 
> > `getThriftFieldId()` inside the constructor?

That's a fine suggestion! It also allowed for single line definitions (at least 
until we have more params there).


> On April 22, 2016, 6:55 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/resources/ResourceTypeConverter.java,
> >  line 37
> > <https://reviews.apache.org/r/46459/diff/3/?file=1357530#file1357530line37>
> >
> >     Shouldn't this be `T value` ?

Unfortunately, it can't. It has to handle `IResource.getRawValue()`, which 
returns an opaque thrift union value. This is the only way to handle type 
conversions in a generic way.


> On April 22, 2016, 6:55 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/migration/V004_CreateTaskResourceTable.java,
> >  line 48
> > <https://reviews.apache.org/r/46459/diff/3/?file=1357535#file1357535line48>
> >
> >     Perhaps to reduce duplication we could use 
> > `ResourceType.<resource>.getAuroraName()` here instead of hard coding in 
> > CPUS, RAM_MB, etc?

It has to be the enum name here, not the `getAuroraName()` but the suggestion 
still makes sense.


- Maxim


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


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