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

storage: short-circuit GC queue when context expires #18642

Merged
merged 1 commit into from
Sep 21, 2017

Conversation

tbg
Copy link
Member

@tbg tbg commented Sep 20, 2017

Fallout from #18199 and corresponding
testing in #15997.

When the context is expired, there is no point in shooting off another gazillion requests.

@tbg tbg requested a review from a team September 20, 2017 21:35
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@bdarnell
Copy link
Contributor

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/storage/gc_queue.go, line 859 at r1 (raw file):

	// succeeded in pushing a lot of transactions, the next time around we should have
	// less work here and manage to get to the intents, but it would be better to let
	// another goroutine push intents for transactions we've handled so that work that

s/push/resolve/


Comments from Reviewable

Fallout from cockroachdb#18199 and corresponding
testing in cockroachdb#15997.

When the context is expired, there is no point in shooting off another gazillion requests.
@tbg
Copy link
Member Author

tbg commented Sep 21, 2017

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/storage/gc_queue.go, line 859 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

s/push/resolve/

Done.


Comments from Reviewable

@nvanbenschoten
Copy link
Member

Reviewed 1 of 2 files at r1.
Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


pkg/storage/gc_queue.go, line 415 at r2 (raw file):

			}(); err != nil {
				// Ignore above error, but if context is expired, no point in keeping going.
				if ctx.Err() != nil {

ctx.Err can return different types of errors, depending on whether the context timed out or was canceled. Why don't we return ctx.Err? Is the idea that err probably already is this ctx.Err? Same question below.


Comments from Reviewable

@tbg
Copy link
Member Author

tbg commented Sep 21, 2017

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/storage/gc_queue.go, line 415 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

ctx.Err can return different types of errors, depending on whether the context timed out or was canceled. Why don't we return ctx.Err? Is the idea that err probably already is this ctx.Err? Same question below.

Yes, and even if it isn't, I think the last error is probably more interesting than a context expired/canceled error.


Comments from Reviewable

@tbg tbg merged commit 763d21e into cockroachdb:master Sep 21, 2017
@tbg tbg deleted the kvdebug/gcq-ctx branch September 21, 2017 17:16
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.

4 participants