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

kvserver,rangefeed: ensure that iterators are only constructed under tasks #52844

Merged

Conversation

ajwerner
Copy link
Contributor

Prior to this change, it was possible for a rangefeed request to be issued
concurrently with shutting down which could lead to an iterator being
constructed after the engine has been closed.

Touches #51544

Release note: None

@ajwerner ajwerner requested a review from andreimatei August 14, 2020 22:45
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/construct-iterator-under-tasks branch from ffa1cc4 to 571f4be Compare August 17, 2020 06:28
Copy link
Contributor

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)


pkg/kv/kvserver/replica_rangefeed.go, line 196 at r1 (raw file):

	}

	// Register the stream with a catch-up iterator.

I think you want to move this comment inside the new function, right?

…tasks

Prior to this change, it was possible for a rangefeed request to be issued
concurrently with shutting down which could lead to an iterator being
constructed after the engine has been closed.

Touches cockroachdb#51544

Release note: None
Copy link
Contributor Author

@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 @andreimatei)


pkg/kv/kvserver/replica_rangefeed.go, line 196 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think you want to move this comment inside the new function, right?

meh, it's the existence of the function that indicates that the stream has been registered with a catch-up iterator.

@ajwerner ajwerner force-pushed the ajwerner/construct-iterator-under-tasks branch from 571f4be to 5cbf775 Compare August 19, 2020 12:24
@ajwerner
Copy link
Contributor Author

TFTR!

bors r=andreimatei

@craig
Copy link
Contributor

craig bot commented Aug 20, 2020

Build succeeded:

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.

3 participants