aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zameer Manji" <zma...@apache.org>
Subject Re: Review Request 30695: Implements log rotation in the Thermos runner.
Date Wed, 29 Jul 2015 20:52:28 GMT


> On Feb. 6, 2015, 10:52 a.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?
> 
> Brian Wickman wrote:
>     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.
> 
> George Sirois wrote:
>     The 'standard' flag makes sense to me, thanks.
>     
>     What do you envision reading the environment variable? The executor/runner? I suppose
we could enhance the scheduler so that you can pass it environment variables to set when launching
the executor so there wouldn't be a lot of plumbing.
>     
>     I guess in general I'm not a huge fan of using the client to enforce basic operational
parameters like this (although I guess it's debatable as to whether or not these settings
qualify :)). For example, it makes it much more challenging to move to a model where jobs
are created/started through native API calls to the scheduler instead of using the client.
> 
> Brian Wickman wrote:
>     Sorry, I totally missed this follow-up comment.
>     
>     If you want to enforce defaults with the client out of the picture, then probably
the best way to do this is to still implement the plumbing as described above but omit Default(Logger,
DefaultLogger), letting it be Empty by default.
>     
>     Add command line parameters to thermos_runner that allow you to toggle which logger
is the default (e.g. --process_logger='rotate' --rotate_log_size=...)
>     
>     With this in place, you can create a new TaskRunnerProvider (TellApartThermosTaskRunnerProvider?
:-) or add flags to the default one that get plumbed through to the aurora_executor command
line.  (e.g. --default_process_logger="rotate")
>     
>     This at least allows you to set organization-wide policy and will be future-proof
if/when the client goes the way of the dodo in favor of a REST API.
> 
> George Sirois wrote:
>     Awesome, thanks. My preference is just for an extra flag to keep things simpler on
our end for now.

George, are you still working on this?


- Zameer


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


On Feb. 6, 2015, 9:51 a.m., George Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30695/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2015, 9:51 a.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
> 
>


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