-
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
feat tools: bucket replicate allows to filter multiple compaction and… #2671
Conversation
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!
Some suggestions (:
pkg/replicate/scheme.go
Outdated
|
||
resolutionMatch := gotResolution == expectedResolution | ||
resolutionMatch := false | ||
for _, allowedResolution := range bf.resolutionLevels { |
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.
What do you think about moving to map? 🤔
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 hope I understood your intentions, is this what you had in mind? 76810cf
fd3995f
to
2dfc46d
Compare
Also I was thinking about moveing to |
76810cf
to
b7453d9
Compare
// TODO(bwplotka): Allow to replicate many compaction levels. | ||
compaction := cmd.Flag("compaction", "Only blocks with this compaction level will be replicated.").Default("1").Int() | ||
resolutions := cmd.Flag("resolution", "Only blocks with these resolutions will be replicated. Repeated flag.").Default("0s", "5m", "1h").HintAction(listResLevel).DurationList() | ||
compactions := cmd.Flag("compaction", "Only blocks with these compaction levels will be replicated. Repeated flag.").Default("1", "2", "3", "4").Ints() |
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.
@bwplotka I eventually took a stab and changed the defaults and the format of --resolution
flag to Go duration.
But if you are against it I'll be happy to roll it back it just felt bit more user friendly.
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 that's ok (:
19ce791
to
bdfcc65
Compare
… resolution lvls Signed-off-by: Martin Chodur <[email protected]>
Signed-off-by: Martin Chodur <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Martin Chodur <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]> Signed-off-by: Martin Chodur <[email protected]>
Signed-off-by: Martin Chodur <[email protected]>
…to use map Signed-off-by: Martin Chodur <[email protected]>
Signed-off-by: Martin Chodur <[email protected]>
… resolution Signed-off-by: Martin Chodur <[email protected]>
bdfcc65
to
1f4ce5e
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.
This looks amazing to me, thanks 👍
// TODO(bwplotka): Allow to replicate many compaction levels. | ||
compaction := cmd.Flag("compaction", "Only blocks with this compaction level will be replicated.").Default("1").Int() | ||
resolutions := cmd.Flag("resolution", "Only blocks with these resolutions will be replicated. Repeated flag.").Default("0s", "5m", "1h").HintAction(listResLevel).DurationList() | ||
compactions := cmd.Flag("compaction", "Only blocks with these compaction levels will be replicated. Repeated flag.").Default("1", "2", "3", "4").Ints() |
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 that's ok (:
Just bumped into this when migrating from one S3 to Swift.
Is this how you imagined it @bwplotka ?
Didn't want to change the defaults even though I'd maybe prefer to replicate all the data by default and make the filtering as blacklist instead of whitelist. But maybe my use-case is different.
Changes
Verification
tested it locally