-
Notifications
You must be signed in to change notification settings - Fork 510
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
Begin converting TenantIndex, BlockMeta, CompactedBlockMeta to proto #3946
base: main
Are you sure you want to change the base?
Conversation
string data_encoding = 14; | ||
uint32 bloom_shard_count = 15; | ||
uint32 footer_size = 16; | ||
repeated tempopb.DedicatedColumn dedicated_columns = 17; |
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.
This causes the encodings to require updates in lots of ways. I'm not sure how to reduce scope.
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.
Perhaps this could be moved off the blockmeta object. Need to check where this is consumed, which could be too many places for us to want here.
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.
Oof. Yes. This is the sticking point. We already have a dedicated columns proto in /pkg/tempopb
.
Wdyt about moving BlockMeta and Dedicated Columns proto to /pkg/tempopb/backend
?
Then the messages in /pkg/tempopb
could easily reference them and that would consolidate all of backend and request proto? or is that just going to cause a bigger mess?
501aa7c
to
baff6f0
Compare
3d37978
to
37abe3e
Compare
What this PR does:
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]