Skip to content
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

Merge upstream at 140f4aa #782

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Merge upstream at 140f4aa #782

merged 2 commits into from
Dec 4, 2024

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Dec 3, 2024

This PR advances this fork closer to upstream Prometheus main, bringing in the changes from prometheus/prometheus@140f4aa, which is the merge of prometheus/prometheus#13567.

This merge had a number of conflicts. While I believe I've merged this correctly given there were minimal conflicts in the tests and all tests are passing, carefully checking these changes would be appreciated.

Conflicts were in:

  • tsdb/block.go
  • tsdb/compact.go
  • tsdb/db.go
  • tsdb/index/index.go
  • tsdb/index/index_test.go
  • tsdb/querier_bench_test.go

yeya24 and others added 2 commits November 11, 2024 07:59
* allow customizing TSDB postings decoder

---------

Signed-off-by: Ben Ye <benye@amazon.com>
…-prometheus

# Conflicts:
#	tsdb/block.go
#	tsdb/compact.go
#	tsdb/db.go
#	tsdb/index/index.go
#	tsdb/index/index_test.go
#	tsdb/querier_bench_test.go
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ charleskorn
❌ yeya24
You have signed the CLA already but the status is still pending? Let us recheck it.

@charleskorn charleskorn marked this pull request as ready for review December 3, 2024 23:51
Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comment (I think I'll just make that a separate PR)
I believe the fact that types changed makes it hard to actually mess this up due to compiler checks. Also index/labelvalues.go seems totally analogous to the upstream changes.

@@ -697,7 +702,7 @@ func (db *DBReadOnly) Blocks() ([]BlockReader, error) {
return nil, ErrClosed
default:
}
loadable, corrupted, err := openBlocks(db.logger, db.dir, nil, nil, nil, DefaultPostingsForMatchersCacheTTL, DefaultPostingsForMatchersCacheMaxItems, DefaultPostingsForMatchersCacheMaxBytes, DefaultPostingsForMatchersCacheForce)
loadable, corrupted, err := openBlocks(db.logger, db.dir, nil, nil, DefaultPostingsDecoderFactory, nil, DefaultPostingsForMatchersCacheTTL, DefaultPostingsForMatchersCacheMaxItems, DefaultPostingsForMatchersCacheMaxBytes, DefaultPostingsForMatchersCacheForce)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this is weird in upstream as well, why would the default be able to decode all? why isn't this coming from options?

func NewFileReader(path string) (*Reader, error) {
return NewFileReaderWithOptions(path, nil)
func NewFileReader(path string, decoder PostingsDecoder) (*Reader, error) {
return NewFileReaderWithOptions(path, decoder, nil)
}

// NewFileReaderWithOptions is like NewFileReader but allows to pass a cache provider and sharding function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here. The doc string wasn't true since #411

Suggested change
// NewFileReaderWithOptions is like NewFileReader but allows to pass a cache provider and sharding function.
// NewFileReaderWithOptions is like NewFileReader but allows to pass a cache provider.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krajorama krajorama merged commit 732248d into main Dec 4, 2024
8 of 9 checks passed
@krajorama krajorama deleted the charleskorn/upgrade-prometheus branch December 4, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants