-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added thanos bucket replicate
#2113
Conversation
Hmm, do we plan to add |
Yep, here is a discuss in the slack. |
Thanks for clarifying 😄 |
07e6c5e
to
1927577
Compare
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.
611a218
to
3a4a697
Compare
thanos bucket replicate
thanos bucket replicate
thanos bucket replicate
1c7ad57
to
2ca1cdb
Compare
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.
Awesome! Few suggestions and LGTM ❤️
Good work!
Also cc @brancz for quick look, as he initially wrote this. 💪
cmd := root.Command("replicate", fmt.Sprintf("Replicate data from one object storage to another. NOTE: Currently it works only with Thanos blocks (%v has to have Thanos metadata).", block.MetaFilename)) | ||
httpMetricsBindAddr, _ := regHTTPFlags(cmd) | ||
toObjStoreConfig := regCommonObjStoreFlags(cmd, "-to", false, "The object storage which replicate data to.") | ||
resolution := cmd.Flag("resolution", "Only blocks with this resolution will be replicated.").Default(strconv.FormatInt(downsample.ResLevel0, 10)).HintAction(listResLevel).Int64() |
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.
hm, I think we might need actually Int64s
for it as we might want to replicate more than one resolution. Let's add TODO
for now.
Also resolution-level
?
resolution := cmd.Flag("resolution", "Only blocks with this resolution will be replicated.").Default(strconv.FormatInt(downsample.ResLevel0, 10)).HintAction(listResLevel).Int64() | |
// TODO(bwplotka): Allow to replicate many compaction levels. | |
resolution := cmd.Flag("resolution-level", "Only blocks with this resolution will be replicated.").Default(strconv.FormatInt(downsample.ResLevel0, 10)).HintAction(listResLevel).Int64() |
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.
Not resolved
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.
In fact it does.
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.
How? I proposed resolution-level
(: I might be wrong, but you should comment that I am wrong, because of X not ignore it. But I am pretty sure you just did not notice (:
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 thought that you mean add more level support for both resolution and compaction, so i added two TODOs here.
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.
Kinpin supports Int
but not Int64
, so still need to use String
here.
Signed-off-by: Xiang Dai <[email protected]>
Signed-off-by: Xiang Dai <[email protected]>
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.
Some comments are still not addressed but was marked as addressed.
cmd := root.Command("replicate", fmt.Sprintf("Replicate data from one object storage to another. NOTE: Currently it works only with Thanos blocks (%v has to have Thanos metadata).", block.MetaFilename)) | ||
httpMetricsBindAddr, _ := regHTTPFlags(cmd) | ||
toObjStoreConfig := regCommonObjStoreFlags(cmd, "-to", false, "The object storage which replicate data to.") | ||
resolution := cmd.Flag("resolution", "Only blocks with this resolution will be replicated.").Default(strconv.FormatInt(downsample.ResLevel0, 10)).HintAction(listResLevel).Int64() |
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.
Not resolved
@bwplotka hi, i do not understand why those codes are old when you review, they were resolved. |
Thank You!
…On Wed, 19 Feb 2020 at 07:48, Xiang Dai ***@***.***> wrote:
@bwplotka <https://github.com/bwplotka> hi, i do not understand why those
codes are old when you review, they were resolved.
Anyway, i have confirmed them again.
For mixin part, i am still working on it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2113?email_source=notifications&email_token=ABVA3O5Q5ANWQVBTPWOYCHLRDTP5JA5CNFSM4KSKAQWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMGWP3I#issuecomment-588081133>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVA3O5CVVU4MJUKM2U6YOTRDTP5JANCNFSM4KSKAQWA>
.
|
Signed-off-by: Xiang Dai <[email protected]>
Can you mark this PR as work in progress until it’s reviewable in its entirety? Thanks! :) |
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.
One thing that bothers me, is the namespacing of the sub-subcommand. I think all the references to this have to be under bucket replicate
from docs, dashboards and alerts. Otherwise, it could create confusion.
thanos bucket replicate
thanos bucket replicate
Signed-off-by: Xiang Dai <[email protected]>
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 !
Signed-off-by: Xiang Dai <[email protected]>
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
thanos bucket replicate
thanos bucket replicate
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.
🎉 Amazing! LGTM ❤️
@@ -29,6 +29,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel | |||
- [#2049](https://github.com/thanos-io/thanos/pull/2049) Tracing: Support sampling on Elastic APM with new sample_rate setting. | |||
- [#2008](https://github.com/thanos-io/thanos/pull/2008) Querier, Receiver, Sidecar, Store: Add gRPC [health check](https://github.com/grpc/grpc/blob/master/doc/health-checking.md) endpoints. | |||
- [#2145](https://github.com/thanos-io/thanos/pull/2145) Tracing: track query sent to prometheus via remote read api. | |||
- [#2113](https://github.com/thanos-io/thanos/pull/2113) Bucket: Added `thanos bucket replicate`. |
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.
It got into wrong release section, so we need to be careful when merging release-0.11 to master @metalmatze
* bucket: Implement replcate Signed-off-by: Xiang Dai <[email protected]> * Document replicate Signed-off-by: Xiang Dai <[email protected]> * feedback Signed-off-by: Xiang Dai <[email protected]> * feedback Signed-off-by: Xiang Dai <[email protected]> * remove version check Signed-off-by: Xiang Dai <[email protected]> * update CHANGLOG Signed-off-by: Xiang Dai <[email protected]> * add chan for SIGHUP Signed-off-by: Xiang Dai <[email protected]> * add existence check Signed-off-by: Xiang Dai <[email protected]> * Add mixin Signed-off-by: Xiang Dai <[email protected]> * rename as replicate Signed-off-by: Xiang Dai <[email protected]> * feedback Signed-off-by: Xiang Dai <[email protected]> * update mixin Signed-off-by: Xiang Dai <[email protected]> * update CHANGLOG Signed-off-by: Xiang Dai <[email protected]> * add bucket prefix Signed-off-by: Xiang Dai <[email protected]>
Signed-off-by: Xiang Dai [email protected]
Implement thanos-replicate to support replicate from one object storage to another.
TODO: