aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 37818: Moved executor settings configuration to loadable JSON
Date Wed, 16 Sep 2015 20:48:44 GMT


> On Sept. 2, 2015, 9:45 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java,
line 87
> > <https://reviews.apache.org/r/37818/diff/3/?file=1061926#file1061926line87>
> >
> >     This code will be easier to maintain long-term with 2 goals in mind:
> >     - keep json-related code within this class (no annotations on ExecutorSettings)
> >     - avoid hand-rolling deserializers
> >     
> >     This sounds like more work, but it's not that bad.  The major difference is
that you'll create a class in this file that defines the JSON structure, and does so in a
way that the json deserializer can work with.
> >     
> >     Maxim just did something very similar, which you can crib from:
> >     https://github.com/apache/aurora/blob/89da936f3d28743e307c7c4fed0bff6fead7ceca/src/main/java/org/apache/aurora/scheduler/SchedulerModule.java#L132-L147
> >     https://github.com/apache/aurora/blob/89da936f3d28743e307c7c4fed0bff6fead7ceca/src/main/java/org/apache/aurora/scheduler/TierManager.java#L47-L55
> 
> Bill Farner wrote:
>     Oh, the detail i neglected to mention - i suggest you then write code to translate
your config schema object into ExecutorSettings.  This can provides compile-time guarantees
that we don't have with the current code, making it easier to maintain.
> 
> Renan DelValle wrote:
>     Can you expend on this? I don't have any experience with Jackson. I'm looking through
what Maxim did and I'll definitely crib here and there but I can't find anything related to
getting this done at compile time.
> 
> Bill Farner wrote:
>     Don't be thrown by the mention of compile time, i'm just referring to refactor safety
if/when the structures changed.  The concise way to put it is - use a different class for
your config domain object.  This way you don't need to change ExecutorSettings to compensate
for the fact that some code in another part of the system is using a json mapper.
>     
>     Start by defining a class that matches your config schema:
>     ```
>     class ExecutorConfig {
>       String command;
>       List<Resources> resource;
>       ...
>     }
>     
>     class Resource {
>       String value;  // note - i would like to see this renamed to uri
>       boolean executable;
>       boolean extract;
>       boolean cache;
>     }
>     ...
>     ```
>     (that's just a quick example, feel free to adjust as you see fit)
>     
>     Jackson is very similar to gson, there's just a behavior nuance that is desirable.
 You'll use an `ObjectMapper` to parse the json to an `ExecutorConfig`, then write a function
to transform your `ExecutorConfig` into `ExecutorSettings`.
>     
>     ```
>     private static ExecutorSettings configToSettings(ExecutorConfig config) {
>       return new ExecutorSettings(...);
>     }
>     ```
> 
> Bill Farner wrote:
>     I took a quick stab at illustrating this, take a look here: https://github.com/wfarner/aurora/commit/a53c215432b34ba95238121581375bde24b88a1e
>     
>     The remaining work is to complete filling out constructor fields:
>     https://github.com/wfarner/aurora/blob/a53c215432b34ba95238121581375bde24b88a1e/src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java#L53-L61
>     
>     This is really a matter of writing functions to translate from types in `ExecutorConfig`
to the corresponding types in `ExecutorSettings`.
>     
>     Depending on your bandwidth over the next few days, i may take over this patch if
you don't mind.
> 
> Renan DelValle wrote:
>     Thanks for the examples, in particular the entry transformer really helped speed
things along.
>     
>     I've almost got this done, and I'm having an issue with using optionals, which in
my opinion are necessary if we want to leave the defaults set on the URI resource configuration
if nothing is specified. I.e: If executable is not in the schema for a URI, then its better
to not set it than to set a default value, IMO.
>     
>     I've looked around and Jackson has support for them if we load a new module:
>     https://github.com/FasterXML/jackson-datatype-jdk8
>     
>     However, I can't find where the jackson dependecy is. And the fact the jackson pacakges
still point to codehaus lead me to believe were using an old version.
>     
>     Here are some relevant files:
>     https://github.com/rdelval/aurora/blob/AURORA-1376-1d/src/main/java/org/apache/aurora/scheduler/configuration/ExecutorConfiguration.java
>     https://github.com/rdelval/aurora/blob/AURORA-1376-1d/src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
>     
>     We're almost there.
> 
> Bill Farner wrote:
>     Perhaps we should go a step further and try to directly parse `ExecutorInfo`?  That
way we don't have to maintain parity between the protobuf schema and our own.  Jackson can't
directly deserialize protobuf-generated objects, but a quick search turned up this promising
project: https://github.com/HubSpot/jackson-datatype-protobuf
>     
>     The deps are defined in build.gradle.  You're right about the old jackson library
(pulled in by an old jersey-json).  However, the fact that they changed packages plays in
our favor - we can add an explicit dep on the new jackson version and use the new one in this
part of the code.
> 
> Bill Farner wrote:
>     I put together a proof of concept of this idea here: https://github.com/wfarner/aurora/commit/04e1ff6ad566755823b624303cfd66541076785b
>     
>     You'll notice that this makes the json more verbose, but it becomes a 1:1 association
with the protobuf message.
> 
> Renan DelValle wrote:
>     Awesome, thanks for that. It's a WIP at the moment, but I just wanted to share what
I have so far so I'm not going off on some trail I shouldn't be going on.
>     
>     I cribbed some of the code from your POC and came up with this. I'm still working
on fixing all the tests that changing the ExecutorSettings broke, but after that it should
be good to go.
>     
>     https://github.com/rdelval/aurora/commit/58e4524ab20698d6c665d7736dadbcac651d445e#diff-ca4de6b05130534619820a5f6a521f2d
>     https://github.com/rdelval/aurora/commit/38958251833566589d448c9f3a19096201430605#diff-ca4de6b05130534619820a5f6a521f2d
>     
>     You'll notice I condensed the command list and URIs into a CommandBuilder and I changed
the Volume from the Aurora one to the Mesos Protobuf one, I hope this last change isn't an
issue, if it is, let me know.
>     
>     With these changes IMO, src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
is no longer needed, at least I haven't found anywhere where it would be a benefit to have
it.
> 
> Bill Farner wrote:
>     The protobuf `Volume` object is the right thing to use.  The most important thing
i would suggest is that you remove `ExecutorSettings`, and let the protobuf go all the way
to where it is used in `TaskInfo`.
>     
>     If CommandUtil is no longer used, it should indeed be removed.
> 
> Renan DelValle wrote:
>     The only thing that concerns me with doing that, is the amount of yak shaving involved
in replacing ExecutorSettings everywhere that it's used in the tests. If you don't mind the
patch ballooing in size a little bit, I also think this is the proper way to do it. Otherwise,
we can leave a transformer for now and do this in a separate patch.

To insulate somewhat from that, you could keep `ExecutorSettings`, but keep `ExecutorInfo.Builder`
as a field inside it.


- Bill


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


On Sept. 8, 2015, 9:32 a.m., Renan DelValle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37818/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2015, 9:32 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This is the first stage in a series of patches to create support for custom executors.
In an effort to expedite the review process, I have decided to break down my patch into multiple
pieces that when/if commited won't break the trunk.
> 
> This patch includes the ability to load configuration from a JSON file. A JSON example
file is included in examples/vagrant/executors-config.json
> 
> Command line arguments have been eliminated and moved over to the JSON file. GSON is
leveraged and does most of the work with the aid of a few custom deserializers that were needed.

> 
> Note that right now a global container mount that does not follow specification will
cause the scheduler to detect the error an exit early. It is up for discussion if this is
the desired behavior or if we should just ignore said mount.
> 
> 
> Diffs
> -----
> 
>   examples/vagrant/executors-config.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 4f43892723db4744db205ea7dd107e9e9ce9d5db

>   examples/vagrant/upstart/aurora-scheduler.conf e909451892f117e9e6eb80994079661827a0914c

>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java c210c0db07bb1f4b3f76668178dcd7e2de56a4ac

>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 197184b6edc0768d677636341b5737f262abdf7d

>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 8047622e206c9827e5cd8e40152a278d495bd0ff

>   src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java aa5ce8b2f14c7dbd0eae120018ee41387c26059f

>   src/main/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoader.java
PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java b3c913892248e4a9a8111412307463985f5ca97f

>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java f6ba2c40aea555d3e0ab774218bfe08d7e1c984b

>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 6fad3344042dc6a75cdf74ce79d388fcd4fc9861

>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 1a25924d789295c5950947f5e302e1d1fbec68f2

>   src/test/java/org/apache/aurora/scheduler/base/CommandUtilTest.java cd0295780d41bc4e914583f195b37eaed28a46dc

>   src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java
PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java dddf7952d3f0e508cd736d5fb95e573267708d43

>   src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java d0987251b058988fcbfab16c1b138c37e0c5b8c6

>   src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
PRE-CREATION 
>   src/test/resources/org/apache/aurora/scheduler/configuration/multiple-executor-example.json
PRE-CREATION 
>   src/test/resources/org/apache/aurora/scheduler/configuration/no-value-URI.json PRE-CREATION

>   src/test/resources/org/apache/aurora/scheduler/configuration/single-executor-example.json
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37818/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build.sh: directory sandbox failed but it may be a flaky test
> bash src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>


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