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
Allow using different listing strategies #7134
Conversation
7cc4098
to
eaa68b7
Compare
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
eaa68b7
to
d5168cf
Compare
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
f424929
to
a972f17
Compare
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
docs/components/compact.md
Outdated
@@ -279,6 +279,15 @@ usage: thanos compact [<flags>] | |||
Continuously compacts blocks in an object store bucket. | |||
|
|||
Flags: | |||
--block-discovery-strategy="concurrent" | |||
One ofconcurrent, recursive. When set to |
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.
nit. missing space
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.
Good catch, thanks 👍
cmd/thanos/store.go
Outdated
@@ -137,6 +146,10 @@ func (sc *storeConfig) registerFlag(cmd extkingpin.FlagClause) { | |||
cmd.Flag("sync-block-duration", "Repeat interval for syncing the blocks between local and remote view."). | |||
Default("15m").DurationVar(&sc.syncInterval) | |||
|
|||
strategies := strings.Join([]string{string(concurrentDiscovery), string(recursiveDiscovery)}, ", ") | |||
cmd.Flag("block-discovery-strategy", "One of"+strategies+". When set to concurrent, stores will concurrently issue one call per directory to discover active blocks in the bucket. The recursive strategy iterates through all objects in the bucket, recursively traversing into each directory. This avoids N+1 calls at the expense of having slower bucket iterations."). |
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 of "+strategies
instead of
"One of"+strategies
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, updated.
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
@@ -754,6 +763,9 @@ func (cc *compactConfig) registerFlag(cmd extkingpin.FlagClause) { | |||
"as querying long time ranges without non-downsampled data is not efficient and useful e.g it is not possible to render all samples for a human eye anyway"). | |||
Default("false").BoolVar(&cc.disableDownsampling) | |||
|
|||
strategies := strings.Join([]string{string(concurrentDiscovery), string(recursiveDiscovery)}, ", ") | |||
cmd.Flag("block-discovery-strategy", "One of "+strategies+". When set to concurrent, stores will concurrently issue one call per directory to discover active blocks in the bucket. The recursive strategy iterates through all objects in the bucket, recursively traversing into each directory. This avoids N+1 calls at the expense of having slower bucket iterations."). |
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.
We revert the current behavior and change back to the old one? Is it worth a changelog?
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 am no authority on anything, but I would personally like to have a changelog entry on this. Seeing how it was mentioned in different threads how this is a blocker for upgrading Thanos itself for some, I think it is important enough.
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.
Good call. I added a changelog entry, let me know if that's sufficient or we should elaborate further.
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 like it. Good enough for me.
cfdf953
to
3f1805b
Compare
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
3f1805b
to
a3e9672
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.
For what its worth, Looks Good To Me. :)
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. Merging now.
Thanks for helping with the review! @mfoldenyi
* Allow using different listing strategies Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Expose flags for block list strategy Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Run make docs Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Fix whitespace Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Add CHANGELOG entry Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> --------- Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
Allow using different listing strategies (thanos-io#7134)
* Allow using different listing strategies Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Expose flags for block list strategy Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Run make docs Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Fix whitespace Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Add CHANGELOG entry Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> --------- Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
In #6474 we changed the way blocks are discovered in object storage by replacing a top-level iteration + exist calls with a single recursive iteration. This method seems to be slow for some users who either have a large amount of objects in the bucket, or prefer faster boot times.
To resolve that problem, this PR exposes a flag allowing users to control the strategy used for discovering TSDB blocks. The default strategy will be one prior to #6474 since it has been there for a long time and has proven to work well for most casesl.
Changes
Verification