changefeedccl: remove changefeed replanning #106595
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
|
This change is |
jayshrivastava
force-pushed
the
remove-replanning
branch
from
ddf36f3 to
074bf5f
Compare
miretskiy
approved these changes
Jul 11, 2023
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jayshrivastava)
-- commits line 29 at r1:
Can you expand this note to indicate that this functionality was disabled by default; and that the change should not impact existing changefeeds in any way.
pkg/ccl/changefeedccl/changefeed_dist.go line 212 at r1 (raw file):
} var replanChangefeedThreshold = settings.RegisterFloatSetting(
you need to add this setting to the retiredSettings list in pkg/settings/registry.go
This commit removes changefeed replanning functionality. Initially, this change was to improve the replanning test, which takes 10 seconds to run on average, preventing us from being able to run it under deadlock (see cockroachdb#101894) or stress race. It turns out that the test did not even test replanning based on range distribution, as it used the `ShouldReplan` testing knob to override replanning decisions. After some discussion with @miretskiy, it was decided that it would be better to remove replanning entirely for these reasons: - it's disabled by default - it's not known to be used by any major production clusters or known to show a significant performance improvement - replanning an entire changefeed on a high level is a large hammer. restarting a large changefeed this way will likely cause rows to be re-emitted which may outweigh the benefit of replanning. if we decide to add replanning, it should be more graceful. - the benefit of replanning is not clear (ie. compared to scalability improvements such as reducing CPU usage, goroutine count etc.) Epic: None Closes: cockroachdb#101894 Release note (enterprise change): This change reverts cockroachdb#83143. This removes the settings `changefeed.replan_flow_frequency` and `changefeed.replan_flow_threshold`. These settings were disabled by default and should not affect existing changefeeds in any way.
jayshrivastava
force-pushed
the
remove-replanning
branch
from
074bf5f to
c436ec5
Compare
|
bors r=miretskiy |
|
Build succeeded: |
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 commit removes changefeed replanning functionality.
Initially, this change was to improve the replanning test, which takes 10 seconds to run on average, preventing us from being able to run it under deadlock (see #101894) or stress race. It turns out that the test did not even test replanning based on range distribution, as it used the
ShouldReplantesting knob to override replanning decisions. After some discussion with @miretskiy, it was decided that it would be better to remove replanning entirely for these reasons:Epic: None
Closes: #101894
Release note (enterprise change): This change reverts #83143. This removes the settings
changefeed.replan_flow_frequencyandchangefeed.replan_flow_threshold. These settings were disabled bydefault and should not affect existing changefeeds in any way.