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