Skip to content
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: do rangefeed enabled check before starting job #41272

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

aayushshah15
Copy link
Contributor

@aayushshah15 aayushshah15 commented Oct 3, 2019

Fixes #41213

If we currently start a changefeed without enabling the
kv.rangefeed.enabled cluster setting, a job first gets started and
then fails when it hits the setting check during the initial Rangefeed
setup.

This PR fixes this by performing this setting check in
changefeedPlanHook in addition to the already existing check in
Replica.Rangefeed.

Release note: CREATE CHANGEFEED statements no longer result in a
superfluous changefeed job being created if rangefeeds are not enabled.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15)


pkg/storage/replica_rangefeed.go, line 131 at r1 (raw file):

	args *roachpb.RangeFeedRequest, stream roachpb.Internal_RangeFeedServer,
) *roachpb.Error {
	if !RangefeedEnabled.Get(&r.store.cfg.Settings.SV) {

There's a sort of crazy edge case where a user upgrades rapidly and there's a rangefeed job which was created on 2.1 (19.1?). It's sort of a bummer because realistically as soon as you've been on 19.2 (or even 19.1?) for long enough for the job to fail it would be fine. That being said, I think you're right that we should return an error where you are.

@ajwerner
Copy link
Contributor

@aayushshah15 want to try to rebase this and get it in?

@aayushshah15
Copy link
Contributor Author

will do, thanks for digging this up again!

@aayushshah15 aayushshah15 force-pushed the changefeed_early_abort branch from f1ec382 to a34f7a4 Compare August 18, 2020 22:00
@aayushshah15 aayushshah15 requested review from a team and adityamaru and removed request for a team and adityamaru August 18, 2020 22:00
@aayushshah15 aayushshah15 force-pushed the changefeed_early_abort branch 2 times, most recently from df97a21 to 6341740 Compare August 19, 2020 13:57
@aayushshah15 aayushshah15 requested a review from ajwerner August 19, 2020 14:02
Fixes cockroachdb#41213

If we currently start a changefeed without enabling the
`kv.rangefeed.enabled` cluster setting, a job first gets started and
then fails when it hits the setting check during the initial `Rangefeed`
setup.

This PR fixes this by performing this setting check in
`changefeedPlanHook` in addition to the already existing check in
`Replica.Rangefeed`.

Release note: None
@aayushshah15 aayushshah15 force-pushed the changefeed_early_abort branch from 6341740 to 7dec9ad Compare August 19, 2020 14:06
@aayushshah15
Copy link
Contributor Author

This is RFAL.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15)

@aayushshah15
Copy link
Contributor Author

TFTR!

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Aug 25, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 25, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 25, 2020

Build succeeded:

@craig craig bot merged commit 7b55c9e into cockroachdb:master Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

changefeedccl: do kv.rangefeed.enabled check before successfully starting job
3 participants