-
Notifications
You must be signed in to change notification settings - Fork 22
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
HDDS-11590. Unexpected character - in Docker Compose env_file variable names #38
Conversation
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.
Overall LGTM. A few high level questions:
- Is
envtoconf.py
being invoked somewhere to act on the variables? Can we test that it is not actually using theozone-site.xml
that is already included in the image? - Do we need a different set of configs for docker compose vs the
ozone-site.xml
already included in the image?
OZONE-SITE.XML_ozone.om.address: "om" | ||
OZONE-SITE.XML_ozone.om.http-address: "om:9874" | ||
OZONE-SITE.XML_ozone.recon.address: "recon:9891" | ||
OZONE-SITE.XML_ozone.recon.db.dir: "/data/metadata/recon" |
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.
Is this config necessary? It looks like the intent is for all other metadata configs to fall back to ozone.metadata.dirs
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.
Probably not necessary, but falling back to ozone.metadata.dirs
produces warning in the log.
And it was present in docker-config
, I just moved and sorted the properties.
|
Ah I was looking at Can we simplify this config process? The whole env variable + python script thing always looked strange to me. IMO the most intuitive way to do this would be:
To me this makes the quick start aspect much more intuitive, because new users won't think that the env var approach is some standard way of configuring Ozone and that the variables are handled by the Ozone process itself. The downside is that the compose file working without other files OOTB is coupled to the config shipped with the Ozone image it is using. Since they are managed in the same repo I feel that this is ok though. If we do want specific service configurations baked into the docker-compose.yaml file (SCM/OM service IDs and membership, for example), passing |
Injecting config via environment variables makes it easy to add or replace some properties. XML files cannot be composed this way: replace
The
Using it only for Ozone is probably overkill in a way. Marton designed it as a uniform way to define configs for images of various services in the ecosystem (Hadoop, Spark, Kafka, etc.). I think fundamentally changing the way configuration is defined is out of scope in this PR. |
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.
Thanks @adoroszlai for this.
I imagine we would want to do the same to compose/*
under ozone
repo later as well?
https://github.com/apache/ozone/tree/master/hadoop-ozone/dist/src/main/compose/ozone
Yes, eventually. |
Thanks for the improvement @adoroszlai. This will be a good step forward to reducing config complexity. I agree that we can discuss further changes to how Ozone handles config input in a separate issue. |
What changes were proposed in this pull request?
Merge configuration from
docker-config
intodocker-compose.yaml
. Benefits:unexpected character "-"
problem that happens with some versions of Docker Compose.https://issues.apache.org/jira/browse/HDDS-11590
How was this patch tested?