> On Aug. 26, 2015, 10: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.
> 
> Kevin Sweeney wrote:
>     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.

Awesome. Thanks for the tips. I just realized that since I'm using a builder 
class to re-enforce all the checks that GSON bypasses with reflection, I can 
add two lines of code in the ExecutorSettings.Builder class and accomplish 
this. Let me know if this will fly.

In ExecutorSettings.Builder, the globalContainerMounts is set to an empty 
immutable list in the constructor. In GSON, if an field isn't found, it's set 
to null.
With these two assumptions in mind in the builder code I changed the code 
From:
```
  public Builder setGlobalContainerMounts(List<Volume> globalContainerMounts) {
      this.globalContainerMounts = globalContainerMounts;
      return this;
    }
```

To:
```
  public Builder setGlobalContainerMounts(List<Volume> globalContainerMounts) {
      if(nonNull(globalContainerMounts)) {
        this.globalContainerMounts = globalContainerMounts;
      }
      return this;
    }
```
This way I don't have to create a new class to get this done. I tested this out 
and both with "globalContainerMounts":[] and globalContainerMounts missing from 
the schema and both return an empty list.


> On Aug. 26, 2015, 10: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.
> 
> Kevin Sweeney wrote:
>     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.

Got it, I understand where you're coming from. This is the last thing to take 
care of before I upload the next patch. I have included a custom field in the 
schema needs to be unpacked. Any suggestions on how I should approach unpacking 
the custom schema? For example, should we create an interface that can be used 
for unpacking the schema and returning a builder of some sort?


- Renan


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


On Aug. 26, 2015, 10: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, 10: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
> 
>

Reply via email to