-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[tempo-distributed] Upgrade to Tempo 2.4.0 #3005
Conversation
great, looking forward for merge |
this lgtm. to be clear i'm not sure that if anyone wants to experiment with these roles to see if tempo does, in fact, use them please do and report back. |
charts/tempo-distributed/values.yaml
Outdated
cache: | ||
caches: | ||
- roles: | ||
- parquet-footer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing parquet-page
but this might be for the best. parquet-page
is incredibly high volume and would forcibly flush out the other values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the warning in the docs, that is why I didn't want to add it as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for reference we run 3 memcached clusters internally:
frontend-search
- small clusterparquet-footer
,parquet-bloom
- medium clusterparquet-pages
- large cluster
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
25456b5
to
f0b8cb3
Compare
Hey @joe-elliott, I've removed |
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
87ebe11
to
5fd216a
Compare
@faustodavid |
Hi @edgarkz , after @joe-elliott comments I changed the code a bit, so I am waiting for their review. CC. @zanhsieh |
Thanks for the ping. Apologies for missing the first. I'll see if i can get a second +1 from someone internally. @faustodavid this is great work, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This is breaking change for anyone using external memcached. It should be a major release with migration instructions. |
Upgrade Tempo to 2.4.0 and use the new multi-cache config.
I have assigned the following roles to the memcache: