-
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
Replicate can specify blocks to copy #3388
Conversation
5e3289c
to
76c27f5
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.
Nice, job! Some suggestions 🤗
cmd/thanos/tools_bucket.go
Outdated
@@ -446,6 +446,8 @@ func registerBucketReplicate(app extkingpin.AppClause, objStoreConfig *extflag.P | |||
Default("0000-01-01T00:00:00Z")) | |||
maxTime := model.TimeOrDuration(cmd.Flag("max-time", "End of time range limit to replicate. Thanos Replicate will replicate only metrics, which happened earlier than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y."). | |||
Default("9999-12-31T23:59:59Z")) | |||
ids := cmd.Flag("id", "Block to be replicated to the destination bucket. If this is specified, then only "+ | |||
"IDs will be used to match blocks and other matchers will be ignored. It only runs once. Repeated field").Strings() |
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.
"IDs will be used to match blocks and other matchers will be ignored. It only runs once. Repeated field").Strings() | |
"IDs will be used to match blocks and other matchers will be ignored. When specified, this command will be run only once after successful replication. Repeated field").Strings() |
pkg/replicate/replicator.go
Outdated
@@ -83,6 +83,7 @@ func RunReplicate( | |||
toObjStoreConfig *extflag.PathOrContent, | |||
singleRun bool, | |||
minTime, maxTime *thanosmodel.TimeOrDurationValue, | |||
blockIDs []string, |
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.
why not pasing ulid.ULID
?
pkg/replicate/scheme.go
Outdated
if _, ok := bf.blockIDs[id]; ok { | ||
return true | ||
} | ||
level.Debug(bf.logger).Log("msg", "filtering block", "reason", "block ID doesn't match", "block_uuid", id) |
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.
This will spam a lot. Maybe we can just not do log it?
pkg/replicate/scheme.go
Outdated
@@ -49,11 +51,16 @@ func NewBlockFilter( | |||
for _, compactionLevel := range compactionLevels { | |||
allowedCompactions[compactionLevel] = struct{}{} | |||
} | |||
blockSet := make(map[string]struct{}) |
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.
The blockID list will be super short. Can we just loop over it instead of using map? Might be simpler (:
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.
Sounds better!
76c27f5
to
4a5af18
Compare
@bwplotka Updated, please take a look again! |
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, just two nits (:
cmd/thanos/tools_bucket.go
Outdated
for _, id := range *ids { | ||
if bid, err := ulid.Parse(id); err != nil { | ||
return errors.Wrap(err, "invalid ULID found in --id flag") | ||
} else { |
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.
no need for else ;p
pkg/compactv2/modifiers_test.go
Outdated
"testing" | ||
) | ||
|
||
func TestIntersection(t *testing.T) { |
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.
Unrelated?
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.
Sorry, will remove this.
Updated again. |
29a0f89
to
8899580
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.
LGTM 💪 rebase only.
8899580
to
e8b3656
Compare
Rebased, PTAL |
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, thank you! Please rebase on master
again and I think we can merge this.
e8b3656
to
b22170c
Compare
Done. PTAL again. |
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
b22170c
to
c2ae9a0
Compare
Just rebased this pr. PTAL again, thanks! |
I will merge this since there are already two approvals! |
Signed-off-by: Ben Ye [email protected]
Changes
Fixes #2713
Added a repeated flag
--id
to specify the blocks to replicate.Verification