-
Notifications
You must be signed in to change notification settings - Fork 820
Define bucket index struct #3553
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
Define bucket index struct #3553
Conversation
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
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.
Nice work!
// in the block, if they match a known pattern. We don't store the full segments | ||
// files list in order to keep the index small. SegmentsFormat is empty if segments | ||
// are unknown or don't match a known format. | ||
SegmentsFormat string `json:"segments_format"` |
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'd suggest to omit empty segments_format and segments_num fields.
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.
Good point!
IndexVersion1 = 1 | ||
|
||
SegmentsFormatUnknown = "" | ||
SegmentsFormat1Based6Digits = "1b6d" |
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.
Can we add a comment on what this format is? This looks quite cryptic without explanation.
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.
Sure
func (m *Block) thanosMetaSegmentFiles() (files []string) { | ||
if m.SegmentsFormat == SegmentsFormat1Based6Digits { | ||
for i := 1; i <= m.SegmentsNum; i++ { | ||
files = append(files, fmt.Sprintf("%06d", i)) |
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.
Nice trick with named return value initialized to nil
.
Blocks []*Block `json:"blocks"` | ||
|
||
// List of block deletion marks. | ||
BlockDeletionMarks []*BlockDeletionMark `json:"block_deletion_marks"` |
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 explicitly called it "Block deletion mark" to avoid misunderstandings with other deletion marks we may have in the system (eg. "Tenant deletion mark").
@@ -68,7 +68,7 @@ type BlocksFinder interface { | |||
|
|||
// GetBlocks returns known blocks for userID containing samples within the range minT | |||
// and maxT (milliseconds, both included). Returned blocks are sorted by MaxTime descending. | |||
GetBlocks(userID string, minT, maxT int64) ([]*BlockMeta, map[ulid.ULID]*metadata.DeletionMark, error) | |||
GetBlocks(ctx context.Context, userID string, minT, maxT int64) (bucketindex.Blocks, map[ulid.ULID]*bucketindex.BlockDeletionMark, error) |
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've introduced context.Context
, even if not used yet, as preliminary step for a follow-up PR.
@@ -170,7 +170,6 @@ func NewBlocksStoreQueryableFromConfig(querierCfg Config, gatewayCfg storegatewa | |||
TenantsConcurrency: storageCfg.BucketStore.TenantSyncConcurrency, | |||
MetasConcurrency: storageCfg.BucketStore.MetaSyncConcurrency, | |||
CacheDir: storageCfg.BucketStore.SyncDir, | |||
ConsistencyDelay: storageCfg.BucketStore.ConsistencyDelay, |
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.
Was unused. Removed it.
Signed-off-by: Marco Pracucci <marco@pracucci.com>
What this PR does:
This is the first PR to introduce the bucket index, as described in this design doc. In this PR I'm just defining the structs and using them in the querier. The logic hasn't changed (unless bugs).
I've also upgraded Thanos (minor changes) as preliminary step for a follow-up PR.
Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]