Skip to content

Conversation

alexisprince1994
Copy link
Collaborator

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-box package. 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 (or DotDict), the dict or DotDict classes will have wonky behavior. Using the Box class 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.toml file located at faktory/config.toml is considered the "base" configuration. This file should always contain every piece of configuration needed to use the client library.

User configuration

The user configuration file should have a similar layout to the base configuration, however it is entirely optional. The user configuration 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 the FAKTORY_USER_CONFIG_PATH environment variable or ~.faktory/config.toml by default.

Environment Variables

Environment variables are parsed with nesting in mind. Outside of the FAKTORY_USER_CONFIG_PATH variable needed to find the user configuration and the FAKTORY_URL variable (used outside of the scope of this PR), the environment variables are expected to have section keys and sub-section keys, all prefixed with FAKTORY. Any variable that does not start with FAKTORY, 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>]__KEY

For example, to set the worker concurrency and server disconnect timeouts respectively:

  • FAKTORY__WORKER__CONCURRENCY
  • FAKTORY__WORKER__DISCONNECT__SERVER_REQUESTED

Overrides

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.

@alexisprince1994 alexisprince1994 requested a review from cdrx April 27, 2020 17:42
@cdrx
Copy link
Owner

cdrx commented Apr 29, 2020

@alexisprince1994 I'm a bit backed up this week; but will review this asap

@alexisprince1994
Copy link
Collaborator Author

@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).

@cdrx
Copy link
Owner

cdrx commented Jun 10, 2020

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

@tswayne
Copy link
Contributor

tswayne commented Jun 21, 2020

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.

@cdrx cdrx marked this pull request as draft April 30, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants