aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 37818: Moved executor settings configuration to loadable JSON
Date Thu, 27 Aug 2015 17:02:21 GMT


> On Aug. 26, 2015, 3:27 p.m., Kevin Sweeney wrote:
> > examples/vagrant/executors-config-new.json, line 18
> > <https://reviews.apache.org/r/37818/diff/1/?file=1055421#file1055421line18>
> >
> >     this isn't a global property - can it be pushed into a custom configuration
object?
> 
> Renan DelValle wrote:
>     We can, but this change causes a ripple effect. If we push this into a custom schema,
we have to provide a way of unpacking custom configuration object into useful mesos data fields
or information usefol to the executor. We need to come to a decision on the best way of doing
this if we go for it. Tangentially, as far as I can tell this is only used when building container
volumes.

As this is greenfield code I'm okay with a bit of a ripple - I'd much rather cluster operators
have to write this config file once than change it as our design for it evolves.


> On Aug. 26, 2015, 3:27 p.m., Kevin Sweeney wrote:
> > examples/vagrant/executors-config-new.json, line 19
> > <https://reviews.apache.org/r/37818/diff/1/?file=1055421#file1055421line19>
> >
> >     It would be ergnomically nice to default this to an empty list if it's left
unset.
> 
> Renan DelValle wrote:
>     I'll see if there's a way of doing that. If you come across anything, let me know.

A bit of Googling suggests that setting defaults in a default constructor works with GSON
here. In fact it seems that just about any way you can think of doing it GSON supports defaults
(confirmed myself here: https://gist.github.com/kevints/d1e26514468863e8c088).

Setting defaults in an initializer works too - I suggest following the pattern in `ImmutableDefaultTest`,
or `ImmutableInitializeOverrideTest` if you have a `@VisibleForTesting` constructor.


- Kevin


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


On Aug. 26, 2015, 3:19 p.m., Renan DelValle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37818/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2015, 3:19 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> 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-new.json PRE-CREATION 
>   examples/vagrant/executors-config.json PRE-CREATION 
>   examples/vagrant/upstart/aurora-scheduler-kerberos.conf 744b4a35c61e749734e222b3d4cbd296927665aa

>   examples/vagrant/upstart/aurora-scheduler.conf 789a3a0315e8530880999432aa9b1e7d0f57d1ff

>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 0c440b5cd5b939872c1ee05d048bf739bfa977cb

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

>   src/test/java/org/apache/aurora/scheduler/configuration/ExecutorSettingsLoaderTest.java
PRE-CREATION 
>   src/test/resources/org/apache/aurora/scheduler/configuration/executor-settings-thermos-no-observer.json
PRE-CREATION 
>   src/test/resources/org/apache/aurora/scheduler/configuration/thermos-settings-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