ref(store): Introduce topic column and age-based drain#726
Open
untitaker wants to merge 10 commits into
Open
Conversation
This file contains hidden or 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
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.
This refactor fixes a few remaining bugs when using multiple topics on
postgres:
When two brokers get assignments
A:1,B:2andB:2,A:1respectively(multi-topic, also sharing a DB), the partition filter is useless for
contention, as each broker filters by
partition in (1, 2). The fix is tointroduce a topic column and make contention management based on
(topic, partition), not justpartition.When a topic gets removed from the configuration, its activations are
orphaned. The fix for this is to extend the partition filter so that rows
older than a minute are not filtered at all.
When we slice pools (a thing we have planned), we will have many broker
deployments with their own DB each (slices), but all sharing the same
consumer group. In this scenario we also want to ensure there are no
orphaned activations due to partitions moving between slices during
rebalancing. This is also fixed by the age-based mechanism, see 1.
Because of these bugs, postgres support for multiple topics was completely
disabled in config validation.
See also https://app.notion.com/p/sentry/Contention-and-draining-in-taskbroker-2-0-37e8b10e4b5d803bad50d2eda7d2d1c6
Changes made:
Add topic column and wire up indices to match old indices. We used to have an
index on (partition), we need to replace that with an index on (topic,
partition). Also update the partition clause to filter on (topic, partition)
Refactor
assign_partitions: Split it up into assign and revoke, so that theconsumer logic becomes simpler. It's easier to pass the full
topic-partition-list from rdkafka directly into the store, and it's more
correct too (in newer rebalancing modes like KIP-848) than to assume
partition revocation clears all partitions.
Update the partition filter to also contain that mentioned age-based clause.
Fix upkeep to actually tag its metrics by topic too, which affects not only
postgres but is also a bug in sqlite.
Remaining issues:
SQLite does not get the topic column, and as a result, all of the upkeep
metrics are still wrong for it. It's not actually necessary to add the topic
column because the three bugs mentioned at the beginning don't exist on
SQLite, and we can live with some busted metrics. It beats having to re-think
SQLite performance. If it weren't for metrics, the partition column could be
removed from sqlite too.
The old
(partition)index still exists in postgres and needs to be removedin a followup once we have confirmed the new index works as expected. I did
some basic tests locally, and it seemed fine.
Some caveats:
When there are old activations that match the age-based clause, the upkeep
metrics could get really weird, since they are tagged by topic and partition.
Not sure what to do about it.
When the topic column is introduced, all existing activations will get
topic: ""backfilled, so when this change is deployed, all existingactivations are "orphaned" until they are drained by age. This affects all
existing postgres pools, of which we have none in production right now.
ref STREAM-1205