Skip to content

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 <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
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").

// 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.

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 <[email protected]>
@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