Skip to content

k8s discovery module: fix issue for druid.host being more than 63chars not permitted as k8s resource label value#10961

Merged
jihoonson merged 3 commits intoapache:masterfrom
himanshug:k8s
Apr 8, 2021
Merged

k8s discovery module: fix issue for druid.host being more than 63chars not permitted as k8s resource label value#10961
jihoonson merged 3 commits intoapache:masterfrom
himanshug:k8s

Conversation

@himanshug
Copy link
Copy Markdown
Contributor

Description

Fixes a bug which shows up in specific deployments, that use k8s based discovery module, where druid.host gets set to something more than 63 characters which is the max length limit on kubernetes label values.

Currently we put full host:port as a k8s label inside pod spec, which can be of arbitrary length. This patch changes node discovery mechanism to work with a hash of host:port in the label value instead.

It is the (2) issue described in #10752 (comment) . Since this issue will be encountered by users quickly so hoping to have this included in 0.21.0 release.

PS: Existing [unit/integration] tests cover the changes made here. It would further be exercised in a larger PR that introduces leader election improvements and integration tests similar to #10680 would be done later, that is blocked on a new release from kubernetes java client lib we use, that contains few leader election algorithm improvements.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • 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.

}
int hash = str.hashCode();
if (hash < 0) {
hash = -1 * hash;
Copy link
Copy Markdown
Contributor

@zhangyue19921010 zhangyue19921010 Mar 9, 2021

Choose a reason for hiding this comment

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

nit: Just wondering if this is a best way to solve 63chars limitation using String.hascode(). As we know, String.hashCode() isn't unique, but it can't be(https://sigpwned.com/2018/08/10/string-hashcode-is-plenty-unique/). And hash = -1 * hash will double the probability of conflict.
In other words, what will happen if there is a conflict? Does the conflict will lead to a wrong SelfDiscovery? If not, I think String.hascode() is good enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hash collision here has no impact on correctness , this is only used to get a suitable list of candidates. and, this is only used for finding a specific node by host:port for the health check.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your explanation :)

Copy link
Copy Markdown
Contributor

@zhangyue19921010 zhangyue19921010 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
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

@jihoonson jihoonson merged commit a0d52c3 into apache:master Apr 8, 2021
jihoonson pushed a commit to jihoonson/druid that referenced this pull request Apr 12, 2021
…s not permitted as k8s resource label value (apache#10961)

* k8s discovery module: fix issue for druid.host being more than 63chars not permitted as k8s resource label value

* update doc

* fix test
jihoonson added a commit that referenced this pull request Apr 13, 2021
…s not permitted as k8s resource label value (#10961) (#11099)

* k8s discovery module: fix issue for druid.host being more than 63chars not permitted as k8s resource label value

* update doc

* fix test

Co-authored-by: Himanshu <g.himanshu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants