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

Rename dns_cache_max_size to dns_cache_max_entries #60500

Merged
merged 10 commits into from
Mar 1, 2024

Conversation

allmazz
Copy link
Contributor

@allmazz allmazz commented Feb 28, 2024

See #60257 (comment)

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Renamed server setting dns_cache_max_size to dns_cache_max_entries to reduce ambiguity

@pufit
Copy link
Member

pufit commented Feb 28, 2024

The CacheBase class here https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/DNSResolver.cpp#L179 takes an optional template argument WeightFunction.

The default weight function is this
https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/ICachePolicy.h#L15

So we need to change either the setting (e.g., dns_cache_max_entries) or add a correct weight function.

This comment was marked as outdated.

src/Core/ServerSettings.h Outdated Show resolved Hide resolved
@rschu1ze
Copy link
Member

rschu1ze commented Feb 28, 2024

The CacheBase class here https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/DNSResolver.cpp#L179 takes an optional template argument WeightFunction.

The default weight function is this https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/ICachePolicy.h#L15

So we need to change either the setting (e.g., dns_cache_max_entries) or add a correct weight function.

Just curious: why? The purpose of this PR is to increase the DNS cache size (see #60257 (comment)), and not change the relative weight of entries.

This comment was marked as duplicate.

3 similar comments

This comment was marked as outdated.

This comment was marked as duplicate.

This comment was marked as duplicate.

@pufit
Copy link
Member

pufit commented Feb 28, 2024

Just curious: why? The purpose of this PR is to increase the DNS cache size (see #60257 (comment)), and not change the relative weight of entries.

IMHO, dns_cache_max_size should limit the actual size of the cache in bytes. Otherwise, the setting's name is very counterintuitive.

@rschu1ze

This comment was marked as outdated.

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2024

CLA assistant check
All committers have signed the CLA.

@allmazz allmazz changed the title increase default value of dns_cache_max_size to 1Mb rename dns_cache_max_size to dns_cache_max_entries Feb 28, 2024

This comment was marked as duplicate.

@allmazz allmazz changed the title rename dns_cache_max_size to dns_cache_max_entries Rename dns_cache_max_size to dns_cache_max_entries Feb 28, 2024

This comment was marked as duplicate.

@pufit pufit added the can be tested Allows running workflows for external contributors label Feb 29, 2024
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-improvement Pull request with some product improvements label Feb 29, 2024
@robot-ch-test-poll1
Copy link
Contributor

robot-ch-test-poll1 commented Feb 29, 2024

This is an automated comment for commit 39457cd with description of existing statuses. It's updated for the latest CI running

⏳ Click here to open a full report in a separate page

Successful checks
Check nameDescriptionStatus
A SyncThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Docs checkBuilds and tests the documentation✅ success
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here✅ success
Mergeable CheckChecks if all other necessary checks are successful✅ success
PR CheckThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success
Unit testsRuns the unit tests for different release types✅ success
Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending

@allmazz allmazz force-pushed the feature/increase_dns_cache_size branch from 5b79b85 to dd4f95e Compare February 29, 2024 03:04
@allmazz allmazz force-pushed the feature/increase_dns_cache_size branch from dd4f95e to 48a4265 Compare February 29, 2024 03:17
@rschu1ze rschu1ze merged commit 2e256b4 into ClickHouse:master Mar 1, 2024
39 of 105 checks passed
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants