feat: Extract task computing into a strategy interface #13690
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
salvacorts
force-pushed
the
salvacorts/blooms/planning-tsdb
branch
from
9613863 to
46e36ca
Compare
github-actions
bot
added
the
type/docs
salvacorts
force-pushed
the
salvacorts/blooms/planning-tsdb
branch
2 times, most recently
from
82bda3c to
55c25a4
Compare
# Conflicts: # pkg/bloombuild/planner/planner_test.go # pkg/validation/limits.go
salvacorts
force-pushed
the
salvacorts/blooms/planning-tsdb
branch
from
55c25a4 to
4c7218b
Compare
chaudum
reviewed
Oct 16, 2024
docs/sources/shared/configuration.md
Outdated
| # Experimental. Bloom planning strategy to use in bloom creation. Can be one of: | ||
| # 'split' | ||
| # CLI flag: -bloom-build.planning-strategy | ||
| [bloom_planning_strategy: <string> | default = "split"] |
There was a problem hiding this comment.
nit, but IMO "split" is too generic for a planning strategy.
I'd rather call it "split_by_series_count", "split_by_series_keyspace", "split_by_series_volume", etc, or short without the "split_by_" prefix.
There was a problem hiding this comment.
Renamed to split_keyspace_by_factor
| @@ -0,0 +1,245 @@ | |||
| package splitkeyspace | |||
There was a problem hiding this comment.
nit: Is is really necessary to make a separate package for each individual strategy?
rfratto
reviewed
Oct 16, 2024
| ctx context.Context, | ||
| table config.DayTable, | ||
| tenant string, | ||
| tsdbs map[tsdb.SingleTenantTSDBIdentifier]common.ClosableForSeries, |
There was a problem hiding this comment.
nit: If we create a type alias we could avoid putting each parameter on its own line:
type TSDBSet = map[tsdb.SingleTenantTSDBIdentifier]common.ClosableForSeries
chaudum
approved these changes
Oct 17, 2024
rfratto
added a commit
to rfratto/loki
that referenced
this pull request
Oct 17, 2024
Use require.ElementsMatch in tests to permit plans having a different order; this is what the tests did prior to refactoring in grafana#13690.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
We currently plan tasks by dividing the FP keyspace into a configurable number of equally-sized splits (bloom_split_series_keyspace_by per-tenant config).
We want to be able to implement new strategies and make them configurable. In this PR we extract the current planning to a new strategy pkg and create a factory to make the strategy configurable.
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR