Skip to content

Docker copy before env and respect JAVA_OPTS#11364

Merged
jihoonson merged 2 commits intoapache:masterfrom
josephglanville:docker-copy-before-env
Oct 8, 2021
Merged

Docker copy before env and respect JAVA_OPTS#11364
jihoonson merged 2 commits intoapache:masterfrom
josephglanville:docker-copy-before-env

Conversation

@josephglanville
Copy link
Contributor

@josephglanville josephglanville commented Jun 13, 2021

This PR introduces two changes to how Druid services are started within the Docker image.

The first changes the behavior of environment variables defined using the druid_ prefix style.
Previously these settings were disregarded when using a config file specified using DRUID_CONFIG_${service}, in effect they were ignored.
With this change environment variables when combined with DRUID_CONFIG_${service} will override the settings in the config file.

The second influences how runtime JVM configuration is calculated when using the Docker image.
Prior to this change settings in a services jvm.config were specified afterJAVA_OPTS on the Java command line when starting the service.
This meant that when a flag is present in both jvm.config and JAVA_OPTS the setting from jvm.config would take precedence, this change reverses this relationship with JAVA_OPTS now being specified after flags sourced from jvm.config

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably needs a mention in the release notes, since it might have some backwards compatibility implications.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also probably worth adding a comment to explain the ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with comment.

@asdf2014 asdf2014 added the Docker https://hub.docker.com/r/apache/druid label Jul 8, 2021
Currently if you provide a config file it negates any settings set via environment variables.
This change allows use of a config file as a base and allow environment variables to override.
Additionally this allows dynamic features such as DRUID_SET_HOST to function correctly when a config file has been provided.
@josephglanville
Copy link
Contributor Author

@xvrl could I get another review of this PR?

Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @josephglanville can you add a section in the PR description that the release manager can use? It will make the life of release manager easier. It would be nice if it mentions potential compatibility issues.

@josephglanville
Copy link
Contributor Author

@jihoonson thanks for the review! I have updated the description with something I think would be adequate for inclusion in the release notes.

@jihoonson
Copy link
Contributor

@josephglanville the updated description LGTM. Thanks!

Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jihoonson jihoonson merged commit 989297e into apache:master Oct 8, 2021
@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docker https://hub.docker.com/r/apache/druid Release Notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants