aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Sirois <jsir...@apache.org>
Subject Re: Review Request 49399: Fix Process log configuration handling.
Date Thu, 30 Jun 2016 17:06:29 GMT


> On June 30, 2016, 7:39 a.m., Stephan Erb wrote:
> > Your fix looks correct. Also thanks for investing the time for the test!
> > 
> > However, I kind of feel that the overall feature is more complicated than it should
be. Two ideas below, feedback welcome.

My thinking is outlined below.
Although I don't think it affects the discussion materially I wanted to point out a premise
I'm working under is that the pystachio schema should be the single source of default values
and enum values.


> On June 30, 2016, 7:39 a.m., Stephan Erb wrote:
> > docs/reference/configuration.md, line 144
> > <https://reviews.apache.org/r/49399/diff/1/?file=1433250#file1433250line144>
> >
> >     Have you considered using `RotatePolicy()` as the default rather than `Empty`?
This could relieve us from having to handle the assigned and unassigned case in the backend,
and thus reduce overall code complexity.

I did. The downside here is that the configuration becomes confusing to read. A user can now
see this in the Observer UI (or the Aurora UI as an escaped json string):
```json
{
    "processes": [
        {
            "daemon": false, 
            "name": "hello", 
            "max_failures": 1, 
            "ephemeral": false, 
            "min_duration": 5, 
            "cmdline": "\n    while true; do\n      echo hello world\n      sleep 10\n   
done\n  ", 
            "logger": {
                "destination": "console", 
                "rotate": {
                    "backups": 5, 
                    "log_size": 104857600
                }, 
                "mode": "standard"
            }, 
            "final": false
        }
    ], 
    "name": "hello", 
    "finalization_wait": 30, 
    "max_failures": 1, 
    "max_concurrency": 0, 
    "resources": {
        "gpu": 0, 
        "disk": 134217728, 
        "ram": 134217728, 
        "cpu": 1.0
    }, 
    "constraints": [
        {
            "order": [
                "hello"
            ]
        }
    ]
}
```

Flags aside (discussed below), the problem would be alleviated by the pystachio schema using
a single `mode = Choice(Standard, Rotate)` field (Choice did not exist when the feature was
written).  Here `Standard` would be an empty `Struct` and `Rotate` would be a rename of `Rotate`
policy, so, in full:
```python
class Logger(Struct):
  destination = Default(LoggerDestination, LoggerDestination('file'))
  mode = Default(Choice(Standard, Rotate), Standard())
```

Perhaps this to allow a deprecation period?:
```python
Rotate = RotatePolicy

class Logger(Struct):
  destination = Default(LoggerDestination, LoggerDestination('file'))
  mode = Default(Choice(LoggerMode, Standard, Rotate), Standard())
  rotate = RotatePolicy
```

The code would remain complex (grow more complex) in the short term while handling old and
new styles, but once the deprecated `rotate` field and `LoggerMode` `Choice` are removed,
the code would then be able to simplify from what exists today.  Perhaps more importantly,
the configuration for end users would simplify - the `mode` field would no longer be ~redundant.


> On June 30, 2016, 7:39 a.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py, lines 125-146
> > <https://reviews.apache.org/r/49399/diff/1/?file=1433251#file1433251line125>
> >
> >     I feel like we can reduce the cyclomatic complexity in `_build_process_logger_args`
if we assign default arugments here, rather than later in the code.

I think this would be less defaulting than you envisioned due to the same odd UI issue as
shown above in tak config json presents in CLI help output.
I could clearly default `--runner-logger-destination` using ~`default=Logger().destination().get()`,
but then you run into both UI and seperation of concerns issues for the next 3.


- John


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


On June 29, 2016, 5 p.m., John Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49399/
> -----------------------------------------------------------
> 
> (Updated June 29, 2016, 5 p.m.)
> 
> 
> Review request for Aurora, George Sirois, Martin Hrabovcin, and Stephan Erb.
> 
> 
> Bugs: AURORA-1724
>     https://issues.apache.org/jira/browse/AURORA-1724
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Previously flagged configuration of Process logging mode would
> blow up and claimed defaulting of the rotation policy did not
> occur.
> 
>  docs/operations/configuration.md                                    |   9 +-
>  docs/reference/configuration.md                                     |  33 ++++----
>  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py |  21 ++---
>  src/main/python/apache/thermos/core/runner.py                       |  40 ++++++---
>  src/main/python/apache/thermos/testing/runner.py                    |  16 ++--
>  src/test/python/apache/thermos/core/BUILD                           |   2 +
>  src/test/python/apache/thermos/core/test_runner_log_config.py       | 230 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 293 insertions(+), 58 deletions(-)
> 
> 
> Diffs
> -----
> 
>   docs/operations/configuration.md e332f864aeb25790f860bc1961a7a2c49b29004e 
>   docs/reference/configuration.md c4b1d387435e4b8fd59a296bd5d6d203bb50cbde 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 203fc47d74840889a1192dc867fef5584b704685

>   src/main/python/apache/thermos/core/runner.py 3ebf86ebd12ed3b68f543d4b9a45615e4681ba7f

>   src/main/python/apache/thermos/testing/runner.py 8b6ba730acedf284a61e4bde60ab480950f92ede

>   src/test/python/apache/thermos/core/BUILD acfb79b6c411744be7f8b3bb364f0c8d049f1694

>   src/test/python/apache/thermos/core/test_runner_log_config.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49399/diff/
> 
> 
> Testing
> -------
> 
> Manually tested the cases in
>   https://issues.apache.org/jira/browse/AURORA-1724.
> 
> Also ran the following green locally:
> ```
> ./pants test src/test/python/apache/thermos/core
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ```
> 
> Doc edits are rendered here:
>   https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/reference/configuration.md#logger
>   https://github.com/jsirois/aurora/blob/jsirois/issues/AURORA-1724/docs/operations/configuration.md#process-logs
> 
> 
> Thanks,
> 
> John Sirois
> 
>


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