Conversation
|
@alexisprince1994 I'm a bit backed up this week; but will review this asap |
|
@cdrx Sounds good! And take your time on this as well, it isn't urgent. I wanted to get your feedback mostly because we'd be adding an external dependency (which normally isn't the biggest deal, but since we currently have zero external dependencies, just wanted to make sure that was fair game). |
|
@alexisprince1994 I'm sorry it has taken me so long to get to this. Quite happy to add dependencies where they are useful and this looks great. Very useful to be able to configure by environment variables -- thank you for this well considered and thorough contribution! |
|
Hi folks, a few comments from another community member. In general I think this change is great. I've built a lot of wrapper code around the client and worker to make a lot of it configurable, so seeing this get added to the library is fantastic. I would however suggest yaml configuration support at least in addition to toml, as yaml has a wider adoption, at least in the dynamic web language ecosystem. Also, just some pr feedback that you can feel free to ignore - there's a lot of added logic in this pr that could probably be pulled from an existing package instead of adding to the scope of this library. You've now introduced maintenance for a detailed (undocumented) toml parser as well as a dot dict utility. |
General
This PR creates the ability to allow for worker (and lays the groundwork for client / connection) defaults to be provided as configuration values instead of hard-coded values in the codebase.
Currently, some values are hard coded, such as timeouts, defaults, etc. While these values may have been chosen intelligently, they allow no room for extension to any user's specific use case or parameterization. If a user wanted to change a default value or update a timeout, they'd need to subclass the appropriate class and re-implement the method, which is less than desirable.
Implementation Details
This PR creates the configuration file,
config.toml, that holds configuration based defaults. These defaults have been hooked into the appropriate classes. To avoid breaking backwards compatibility, the values in the configuration files are set to what the hard-coded values were.Dependencies
Prior to this PR, the client library had no external dependencies (outside of development & testing ones). This PR introduces one dependency on the
python-boxpackage. This is to help with the dynamic loading of configuration and making sure that proper keys are not overriden. For example, if a user in their user config (explained more in detail below) overrode a method of a dictionary (orDotDict), thedictorDotDictclasses will have wonky behavior. Using theBoxclass will allow us to both have more predictable behavior on loading conflicting keys but not need to worry about maintaining a third party version of it.Configuration Overriding
Default / Base configuration
The
config.tomlfile located atfaktory/config.tomlis considered the "base" configuration. This file should always contain every piece of configuration needed to use the client library.User configuration
The
userconfiguration file should have a similar layout to the base configuration, however it is entirely optional. Theuserconfiguration file should be used to either extend the values present, overwrite when it makes sense, or both. The individual user can programmatically decide where they are storing this configuration file based on theFAKTORY_USER_CONFIG_PATHenvironment variable or~.faktory/config.tomlby default.Environment Variables
Environment variables are parsed with nesting in mind. Outside of the
FAKTORY_USER_CONFIG_PATHvariable needed to find the user configuration and theFAKTORY_URLvariable (used outside of the scope of this PR), the environment variables are expected to havesectionkeys andsub-sectionkeys, all prefixed withFAKTORY. Any variable that does not start withFAKTORY, the item is not read into the configuration. This helps avoid reading in any unnecessary sensitive information exposed in the environment for other reasons.To load data from the environment under a specific key, the following format is expected:
FAKTORY__<SECTION>__[...<SUBSECTIONS>]__KEYFor example, to set the worker concurrency and server disconnect timeouts respectively:
FAKTORY__WORKER__CONCURRENCYFAKTORY__WORKER__DISCONNECT__SERVER_REQUESTEDOverrides
Given the 3 options above for setting configuration variables, the override priority goes environment variable > user config > base config. This allows the users to have a customizable configuration based on multiple pieces of information.
Future Work
In the future, I could see further extensions to what options are configurable. If there are connection aspects that can be configured for better performance with certain workloads or certain consistency guarantees (someone who really needs to crank the performance out of the workers), we'd like to enable that for their specific use case while still retaining our "recommended" configuration options via the "base" configuration.