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

feat: rename DisableTraceLocality to TraceCache #1450

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Nov 27, 2024

Which problem is this PR solving?

To improve readability of the config option for our users, this PR changes the binary trace locality feature flag to a mode structure.

Short description of the changes

  • Rename DisableTraceLocality to TraceCache that comes with two modes, concentrated and distributed

@VinozzZ VinozzZ added the type: enhancement New feature or request label Nov 27, 2024
@VinozzZ VinozzZ added this to the v2.9 milestone Nov 27, 2024
@VinozzZ VinozzZ self-assigned this Nov 27, 2024
@VinozzZ VinozzZ requested a review from a team as a code owner November 27, 2024 17:23
@kentquirk
Copy link
Contributor

I like the idea here a lot, but I am not so sure about the name because we have had other things about trace caches in the past and I think it's confusing. What do you think of something like TraceLocalityMode?

Copy link
Contributor

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks good, just one typo that will need to have docs regenerated again.

config/metadata/configMeta.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

I had some minor documentation tweaks, but this is good.

refinery_config.md Outdated Show resolved Hide resolved
refinery_config.md Outdated Show resolved Hide resolved
@VinozzZ VinozzZ force-pushed the yingrong/trace_cache_config_option branch from 96a86bc to 43dd1a8 Compare December 2, 2024 16:41
@VinozzZ VinozzZ merged commit 5ce285d into main Dec 2, 2024
5 checks passed
@VinozzZ VinozzZ deleted the yingrong/trace_cache_config_option branch December 2, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants