> On June 30, 2016, 3:39 p.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. > > John Sirois wrote: > 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.
Thanks for the thoughtful response. I like your proposed deprecation plan. - Stephan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49399/#review140156 ----------------------------------------------------------- On June 30, 2016, 1 a.m., John Sirois wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49399/ > ----------------------------------------------------------- > > (Updated June 30, 2016, 1 a.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 > >
