> 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?
> 
> 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.
> 
> Zameer Manji wrote:
>     George, are you still working on this?

Yeah, sorry for the delay. I'll have it cleaned up and in review this week.


- George


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