Skip to content
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

Use canonical hostname instead of ip by default #16386

Merged

Conversation

aho135
Copy link
Contributor

@aho135 aho135 commented May 3, 2024

Fixes #16352

Description

druid.indexer.task.restoreTasksOnRestart does not work by default for Docker based deployments on Kubernetes

Updates druid.sh to use canonical hostname for druid.host by default. Allows users to set environment variable DRUID_SET_HOST_IP=1 to use IP instead

Release note


Key changed/added classes in this PR
  • druid.sh

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@FrankChen021 FrankChen021 added Area - Operations Release Notes Docker https://hub.docker.com/r/apache/druid labels May 6, 2024
@FrankChen021
Copy link
Member

This changes the default behavior, need more people to review it.
IMO, in K8S deployment, it's reasonable to use host name instead of ip, changing to host name by default makes sense.

@@ -138,8 +138,8 @@ then
setKey _common druid.zk.service.host "${ZOOKEEPER}"
fi

DRUID_SET_HOST=${DRUID_SET_HOST:-1}
if [ "${DRUID_SET_HOST}" = "1" ]
DRUID_SET_HOST_IP=${DRUID_SET_HOST_IP:-0}
Copy link
Contributor

Choose a reason for hiding this comment

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

@georgew5656 could you please review this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aho135 does this new default work okay with a raw docker run? i'm not exactly sure what
InetAddress.getLocalHost().getCanonicalHostName() returns there? i think it might be the container-id which might not work that well since its not reachable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@georgew5656 What did you have in mind as a raw docker run? The integration tests docker setup also uses InetAddress.getLocalHost().getCanonicalHostName() which just resolves to the name set in the docker-compose

Copy link
Contributor

Choose a reason for hiding this comment

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

if the integration tests are passing then this seems fine to me

@aho135
Copy link
Contributor Author

aho135 commented May 10, 2024

Thanks for the review and approval @georgew5656! Do you know who would be the right person to tag for review and merge?

@georgew5656
Copy link
Contributor

Thanks for the review and approval @georgew5656! Do you know who would be the right person to tag for review and merge?

i'd say if @FrankChen021 also thinks this is good to go I can merge it

@FrankChen021 FrankChen021 merged commit 9459722 into apache:master May 11, 2024
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Operations Docker https://hub.docker.com/r/apache/druid Release Notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

druid.indexer.task.restoreTasksOnRestart does not work by default for Docker based deployments on Kubernetes
4 participants