-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Enable configuring redis in docker setup #29499
base: master
Are you sure you want to change the base?
Conversation
RESULTS_BACKEND = RedisCache( | ||
key_prefix="superset_results", | ||
ssl=REDIS_PROTO == "rediss", | ||
password=REDIS_PASSWORD or None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: couldn't you just put None
inside .getenv
call as the default?
REDIS_PASSWORD = os.getenv("REDIS_PASSWORD", None)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This construct is there to cast the empty string to None to disable the password (as cachelib documents password as str | None
), but technically it's not necessary as eventually redis-py will cast None back to the empty string anyway (here) so this will do:
password=REDIS_PASSWORD or None, | |
password=REDIS_PASSWORD, |
Not applying the change for now pending the discussion about whether or not the PR is something that should be merged in the first place.
Are you secretly trying to use our docker-compose setup in production!? :) I'm not against making the dev setup more configurable, but the more we push in that direction, the harder it is to guarantee an simple Why not simply improving the docs as to how to configure Redis in any superset_config file (production or dev), and clarifying how to alter docker-compose using About the idea of exposing more |
Guilty as charged :) I'm not quite at the stage where moving to k8s makes sense but the self-contained nature of container still adds a ton of value so docker-compose has been wonderful.
I hear you. The suggestion definitely works in environments where you have access to a filesystem for setting up a volume mount for the custom config (which isn't always the case, e.g. many cloud offerings that will run your docker image for you won't let you easily do volumes) or it forces you to build your own image that bakes in the custom config (which adds a fair bit of friction on top of being able to just deploy the official image with some environment variable changes). With the changes proposed in this PR, however, the official image turns into something that can be easily configured to externalize all of its state which IMO has value and extends the possible use-cases. |
Ah! I've been having this idea to expose more of what's in The idea would be to allow-list at set of keys, and look for matching |
I think I had or someone else had done something similar with Airflow a while back. Reading at what it looks like today -> https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html, there are good ideas in there, like sections and conventions. |
SUMMARY
This PR adds some configuration options for the docker-compose setup:
These changes are useful in scenarios where we want to use the docker-compose setup together with an existing redis instance and when we potentially want to run the superset UI on a different node from the superset workers without going all the way to using the k8s setup.
TESTING INSTRUCTIONS
For testing the redis password support, apply the following patch:
Now start the stack via
TAG=4.0.2 docker compose -f docker-compose-image-tag.yml up
and verify that Redis operations work.For testing the redis results backend support, toggle the feature via
echo RESULTS_BACKEND_CACHE_TYPE=RedisCache > docker/.env-local
, restart the stack, and verify that Redis operations work.ADDITIONAL INFORMATION