-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
streamingccl: prevent node lag replanning starvation #114525
Conversation
5a6084f
to
68c90d2
Compare
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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.
Aha, thanks for looking into this. Left some nits about the logging.
pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go
Outdated
Show resolved
Hide resolved
pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go
Outdated
Show resolved
Hide resolved
pkg/ccl/streamingccl/streamingest/stream_ingestion_frontier_processor.go
Outdated
Show resolved
Hide resolved
This patch prevents the lastNodeLagCheck time from updating every time the frontier processor receives a checkpoint, which can happen every few seconds. This previously prevented the node lag replanning check to trigger because this time needed to be older than 10 minutes. Rather, this timestamp should only update if we actually compute the lag check. Fixes cockroachdb#114341 Release note: none
68c90d2
to
b61cbb9
Compare
TFTR! bors r=stevendanna |
Build succeeded: |
This patch prevents the lastNodeLagCheck time from updating every time the frontier processor receives a checkpoint, which can happen every few seconds. This previously prevented the node lag replanning check to trigger because this time needed to be older than 10 minutes. Rather, this timestamp should only update if we actually compute the lag check.
Fixes #114341
Release note: none