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

server: use longer-lived context for purgeOldSessions #90693

Closed

Conversation

stevendanna
Copy link
Collaborator

It looks like, the async task start by startPurgeOldSessions outlived the span on the context that it was being passed. Here, we pass it an annotated background context.

Fixes #90692

Epic: None

Release note: None

It looks like, the async task start by startPurgeOldSessions outlived
the span on the context that it was being passed.  Here, we pass it an
annotated background context.

Fixes cockroachdb#90692

Epic: None

Release note: None
@stevendanna stevendanna requested a review from a team October 26, 2022 12:31
@stevendanna stevendanna requested review from a team as code owners October 26, 2022 12:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@stevendanna
Copy link
Collaborator Author

Meant to open this as a draft, this is untested.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

This is likely not sufficient. This PR should import 82e2456 (the 3rd commit from #90608).

@knz
Copy link
Contributor

knz commented Oct 26, 2022

That said, Andrei and I figured out that the cancel function for the ctx under scrutiny here is not called before the server shuts down, so there is no bug.

edit I was wrong

@stevendanna
Copy link
Collaborator Author

@knz Looks like your PR covers this and more. If you want to fold in the change to tenant.go into your PR (assuming it is correct), we could just merge yours and close this.

@knz
Copy link
Contributor

knz commented Oct 26, 2022

I'd prefer if you could extract that commit here (and possibly do the change in tenant.go as well), so we can backport your PR and #90523 together as a lighterweight change to 22.2, as requested by @rafiss .

@stevendanna
Copy link
Collaborator Author

@knz I pushed another commit that changes other uses of ctx in startTenantInternal to background. Let me know if any of those don't look correct to you.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

cheers

@stevendanna
Copy link
Collaborator Author

This hit some CI issues so I couldn't merge it before going offline yesterday. It looks like most of these fixes got merged in other PRs. We'll just need to remember to echo some of those fixes in any backport.

I think the makePlanner thing still needs to be fixed. I'll open a PR for that.

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.

server: tracing span use after finish
3 participants