-
Notifications
You must be signed in to change notification settings - Fork 797
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
Update Thanos to latest main #4585
Conversation
Update Thanos dependency to include thanos-io/thanos#4928, to conserve memory. Signed-off-by: Andrew Seigner <andrew@sig.gy>
Thanks for doing this. It's best to cover all important changes when updating a dependency, so I checked Please can you check the list and summarise all user-visible changes in the CHANGELOG. Just in a few words - it's not necessary to link to individual PRs besides this one where a keen reader can find all the details. Readers of CHANGELOG are assumed to be admins who don't want code-level details. |
@@ -86,8 +83,8 @@ replace google.golang.org/grpc => google.golang.org/grpc v1.38.0 | |||
// We only pin this version to avoid problems with running go get: github.com/thanos-io/thanos@main. That | |||
// currently fails because Thanos isn't merging release branches to main branch, and Go modules system is then | |||
// confused about which version is the latest one. v0.22.0 was released in July, but latest tag reachable from main | |||
// is v0.19.1. We pin version from late september here. Feel free to remove when updating to later version. | |||
replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211122085937-de0e3848ff60 | |||
// is v0.19.1. We pin version from early December here. Feel free to remove when updating to later version. |
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.
Question: is the pin still necessary?
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.
Thanos release tags are still not on the main
branch. This replace
pattern allowed me to simply put main
on this line, and then go mod vendor
correctly resolved it to v0.19.1-0.20211122085937-de0e3848ff60
.
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.
Thanks.
The comment above says it's about merging release branches, but this kinda looks like a merge of the release branch: thanos-io/thanos#4738
Did it not work? Not what Go is expecting?
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.
That was a squash merge, not merge commit. It didn't make tag v0.23.1
reachable from main
branch.
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 tried to update release process instructions in Thanos at thanos-io/thanos#4770 to hopefully fix this situation in the future.
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.
Thanks for filing thanos-io/thanos#4770 @pstibrany!
Did it not work? Not what Go is expecting?
So we can do a go get
on a release tag, and it works:
$ go get github.com/thanos-io/thanos@v0.24.0-rc.1
$ go mod vendor
$ git diff
...
- github.com/thanos-io/thanos v0.22.0
+ github.com/thanos-io/thanos v0.24.0-rc.1
-replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211208205607-d1acaea2a11a
+// replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211208205607-d1acaea2a11a
... I don't know how important it is that we be able to do go get github.com/thanos-io/thanos@main
, but that still doesn't work:
$ go get github.com/thanos-io/thanos@main
go: downloading github.com/thanos-io/thanos v0.19.1-0.20211213141611-c7a44e23febe
go get: github.com/thanos-io/thanos@main (v0.19.1-0.20211213141611-c7a44e23febe) requires github.com/thanos-io/thanos@v0.22.0, not github.com/thanos-io/thanos@main (v0.19.1-0.20211213141611-c7a44e23febe)
So I guess we have a few options:
- Leave as-is, wait for Add step for merging release branch back to main. thanos-io/thanos#4770.
- Remove the
replace
and the comment,require
v0.24.0-rc.1
directly. - Remove the
replace
and the comment, wait forv0.24.0
andrequire
it directly.
I think this would need plumbing. Cache instantation in Cortex: cortex/pkg/storage/tsdb/caching_bucket.go Lines 152 to 169 in 4474da8
cortex/pkg/storage/tsdb/index_cache.go Lines 79 to 89 in 4474da8
... would need to call cacheutil.NewRedisClient in Thanos:https://github.com/thanos-io/thanos/blob/9a26b0edee19a06c6e99a09e33ebceca734c91f9/pkg/cacheutil/redis_client.go#L122-L133 Interestingly the Thanos PR mentions Cortex's redis support as a reference implementation: thanos-io/thanos#4888 |
Signed-off-by: Andrew Seigner <andrew@sig.gy>
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.
Thanks; LGTM
@@ -86,8 +83,8 @@ replace google.golang.org/grpc => google.golang.org/grpc v1.38.0 | |||
// We only pin this version to avoid problems with running go get: github.com/thanos-io/thanos@main. That | |||
// currently fails because Thanos isn't merging release branches to main branch, and Go modules system is then | |||
// confused about which version is the latest one. v0.22.0 was released in July, but latest tag reachable from main | |||
// is v0.19.1. We pin version from late september here. Feel free to remove when updating to later version. | |||
replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211122085937-de0e3848ff60 | |||
// is v0.19.1. We pin version from early December here. Feel free to remove when updating to later version. |
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.
Thanks.
The comment above says it's about merging release branches, but this kinda looks like a merge of the release branch: thanos-io/thanos#4738
Did it not work? Not what Go is expecting?
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.
LGTM, thanks!
* Update Thanos to latest main Update Thanos dependency to include thanos-io/thanos#4928, to conserve memory. Signed-off-by: Andrew Seigner <andrew@sig.gy> * Update changelog to summarize user-facing changes Signed-off-by: Andrew Seigner <andrew@sig.gy> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
Update Thanos dependency to include thanos-io/thanos#4928, to conserve memory.
Signed-off-by: Andrew Seigner andrew@sig.gy
What this PR does:
Bumps Thanos dependency to the latest main: thanos-io/thanos@d1acaea
Most notably this pulls in thanos-io/thanos#4928, which gives a 2x~4x memory reduction when using Azure blocks storage:
This PR, combined with #4581, brings Cortex's memory usage back inline with
v1.10.0
.Relates to:
http
config settings in Azure storage #4581Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]