> 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 > >
