> On Feb. 6, 2015, 6:52 p.m., Brian Wickman wrote:
> > This is super rad.  Thanks for taking this on.
> > 
> > Before I do a deeper dive, what do you think about making the logrotate 
> > policy be specified by the user instead of the framework owner, with a 
> > sensible default?  For example, if this is configurable on the process 
> > object, you can have different policies per process, e.g.
> > 
> > ```py
> > class RotatePolicy(Struct):
> >   log_size = Default(Integer, 32*MB)
> >   backups = Default(Integer, 10)
> >   copytruncate = Default(Boolean, False)
> >   compress = Default(Boolean, False)
> >   hangup_command = String
> >   ...
> >     
> > # union
> > class Logger(Struct):
> >   standard = Boolean  # standard i/o
> >   devnull = Boolean   # /dev/null redirection
> >   logrotate = RotatePolicy  # use a logrotation policy
> >     
> > DefaultLogger = Logger(standard=True)
> >     
> > class Process(Struct):
> >   cmdline = Required(String)
> >   name    = Required(String)
> >   ...
> >   logger  = Default(Logger, DefaultLogger)
> > ```
> > 
> > This also means reduced end-to-end plumbing through all the binaries, class 
> > constructors, etc.  And if you ever need to add new features (e.g. a 
> > compress option), they're fairly well encapsulated within the Logger union.
> 
> George Sirois wrote:
>     Awesome, thanks for the feedback.
>     
>     I'd be willing to take this on; it would definitely make the plumbing a 
> lot cleaner and provide more flexibility, although the downside is that it's 
> now harder to apply a universal default (besides whatever we arrive at as the 
> Aurora default).
>     
>     I'll be able to pick this up next week and can probably have a modified 
> review out by Wednesday evening. What do you think about starting out with a 
> simple configuration (just log_size and backups on RotatePolicy) and 
> iterating from there? 
>     
>     I also have one question - what distinction are you making between the 
> "standard" flag on Logger and the existence of a rotation policy?

Yeah, all the extra parameters were just for illustration only.  Not asking for 
any more functionality than what you already have since it already provides 
tremendous value.

The idea for 'standard' in Logger is just to be explicit about current behavior 
(unrestricted logging to stdout/stderr) and use it as the default.

As for applying a universal default that's not "standard", there are a few ways 
that you could do this, from environment variables (THERMOS_FORCE_ROTATE? idk) 
to building an aurora client using a custom entry point that patches 
Process.TYPEMAP['logger'] to use a different default.  Both are kind of sketch 
but within the realm of sketch found elsewhere in the code.


- Brian


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


On Feb. 6, 2015, 5:51 p.m., George Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30695/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2015, 5:51 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Brian Wickman.
> 
> 
> Bugs: AURORA-95
>     https://issues.apache.org/jira/browse/AURORA-95
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Implements log rotation in the Thermos runner.
> 
> 
> Diffs
> -----
> 
>   docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 0752d50015b2ff936f079c4a9f2777172dc00a93 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 
> 9ff8c5379aad7ac011115e44b1f5a2b74f759f26 
>   src/main/python/apache/thermos/bin/thermos_runner.py 
> bd8cf7f4cda54b6be72dad64f9446eedeb132211 
>   src/main/python/apache/thermos/core/process.py 
> 5ce138dab161d880c0bd58b87a6f5a54d4ca2f99 
>   src/main/python/apache/thermos/core/runner.py 
> f949f279a071c6464b026749f51afc776102f2aa 
>   src/test/python/apache/thermos/core/test_process.py 
> e261249b977802851ffc3d89437761c532fcd3f8 
> 
> Diff: https://reviews.apache.org/r/30695/diff/
> 
> 
> Testing
> -------
> 
> ./pants test src/test/python/apache/thermos/core:all
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Reply via email to