-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
changefeedccl: check that rangefeeds are enabled earlier #70130
changefeedccl: check that rangefeeds are enabled earlier #70130
Conversation
"CHANGEFEED", | ||
); err != nil { | ||
return err | ||
} |
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'm moving this down into the returned PlanHookRowFn
based on this discussion: https://cockroachlabs.slack.com/archives/C9TGBJB44/p1631200940138700
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 for tagging me to review, TIL about the PlanHookRowFn
difference
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.
Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)
Pushing this check earlier means that for core changefeeds, the check happens before the backfill. Previously an error would only be seen after the backfill when we attempted to start the rangefeed, which some users found confusing. Release note: None
55d9096
to
e372d9e
Compare
bors r=miretskiy |
This PR was included in a batch that was canceled, it will be automatically retried |
Build succeeded: |
Pushing this check earlier means that for core changefeeds, the check
happens before the backfill. Previously an error would only be seen
after the backfill when we attempted to start the rangefeed, which
some users found confusing.
Release note: None