Skip to content

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

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Dec 1, 2020

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

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Contributor

@pstibrany pstibrany left a 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"`
Copy link
Contributor

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.

Copy link
Contributor Author

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"
Copy link
Contributor

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.

Copy link
Contributor Author

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))
Copy link
Contributor

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"`
Copy link
Contributor Author

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)
Copy link
Contributor Author

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,
Copy link
Contributor Author

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>
@pracucci pracucci merged commit a775841 into cortexproject:master Dec 1, 2020
@pracucci pracucci deleted the define-bucket-index-struct branch December 1, 2020 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants