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.

Sign up for GitHub

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

Jump to bottom

catalog/lease: avoid using context.Background directly #72638

Merged
merged 3 commits into from
Nov 11, 2021
Merged

catalog/lease: avoid using context.Background directly #72638

merged 3 commits into from
Nov 11, 2021

Conversation

knz
Copy link
Contributor

@knz knz commented Nov 11, 2021 •
edited
Loading

First commit from #72651
Informs #58938.

  • (*AmbientContext).AnnotateCtx() - takes care of connecting the
    context to the tracer
  • logtags.FromContext / logtags.WithTags - reproduces the logging
    tags on the child context.

Release note: None

@knz knz requested review from ajwerner, tbg and a team November 11, 2021 12:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Nov 11, 2021

@tbg is there some pre-defined API in the stopper that would simplify/automate this pattern?

@knz knz changed the title catalog/lease: avoid using context.Background catalog/lease: avoid using context.Background directly Nov 11, 2021
ajwerner
ajwerner approved these changes Nov 11, 2021
View reviewed changes
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 4 of 4 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

@tbg
Copy link
Member

tbg commented Nov 11, 2021

No, we don't. Isn't the eventual end point here that we'd lint against use of context.Background and make the Stopper own the background context? So you would write s.RunAsyncTask(s.Background(), ...).

This was referenced Nov 11, 2021
Merged
Merged
Closed
@knz
Copy link
Contributor Author

knz commented Nov 11, 2021

Isn't the eventual end point here that we'd lint against use of context.Background and make the Stopper own the background context?

Something like that, yes.
Before we go there however I want to get more experience with this pattern to see if it fits our other use cases. Once I have a sequence of PRs all doing something similar, I'll track back and do an abstraction.

@knz knz requested a review from a team November 11, 2021 14:52
@knz
Copy link
Contributor Author

knz commented Nov 11, 2021

TFYR

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Nov 11, 2021

Build failed (retrying...):

@knz
Copy link
Contributor Author

knz commented Nov 11, 2021

bors r-

@craig
Copy link
Contributor

craig bot commented Nov 11, 2021

Canceled.

knz added 3 commits November 11, 2021 18:43
@knz
jobs: follow pattern to use the background context
8d69380 
Release note: None
@knz
catalog/lease: avoid using context.Background
e8061cb 
- `(*AmbientContext).AnnotateCtx()` - takes care of connecting the
  context to the tracer
- `logtags.FromContext` / `logtags.WithTags` - reproduces the logging
  tags on the child context.

Release note: None
@knz
sql: ensure that descriptor lease drains are traced
ca0df0f 
Release note: None
@knz
Copy link
Contributor Author

knz commented Nov 11, 2021

TFYR

bors r=ajwerner

@craig craig bot merged commit 4300949 into cockroachdb:master Nov 11, 2021
@craig
Copy link
Contributor

craig bot commented Nov 11, 2021

Build succeeded:

@knz knz deleted the 20211111-leases branch November 12, 2021 10:17
@knz knz mentioned this pull request Nov 16, 2021
Merged
craig bot pushed a commit that referenced this pull request Nov 18, 2021
@tbg @knz
Merge #72471 #72836
e1c7d6a 
72471: kvserver: fix bugs in & fortify tenant refcounting r=ajwerner a=tbg

This PR fixes a sandwich of two bugs around refcounting the tenant rate limiters and metrics that I found while prototyping around #72374.

We had an accidental early return in `postDestroyRaftMuLocked` that meant that tenant metrics and rate limiters were essentially never released.

We were also continuing to use at least the tenant metrics object after the call to `postDestroyRaftMuLocked` had returned (but note that the above bug meant that we hadn't actually released the ref).

This PR fixes both and adds precautions against regressions of such bugs.

Despite having fixed bugs, I would be cautious about backporting this to 21.2 and 21.1; the bugs here never seem to have caused any problems, and since our day-to-day testing isn't heavy on tenants, it's unclear how reliably we'd be shaking out problems that were previously masked by the bug.

72836: server,sql,kv: various context improvements r=miretskiy,tbg a=knz

Informs #58938.

Connects more async goroutines to the tracer.

Also fixes various defects I introduced in #72638 and #72605.


Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajwerner ajwerner ajwerner approved these changes

@tbg tbg Awaiting requested review from tbg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knz @cockroach-teamcity @tbg @ajwerner