Skip to content

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

Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom
knz:20211111-leases
Nov 11, 2021
Merged

catalog/lease: avoid using context.Background directly#72638
craig[bot] merged 3 commits intocockroachdb:masterfrom
knz:20211111-leases

Conversation

@knz
Copy link
Copy Markdown
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 a team, ajwerner and tbg November 11, 2021 12:28
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Nov 11, 2021

This change is Reviewable

@knz
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

knz commented Nov 11, 2021

TFYR

bors r=ajwerner

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 11, 2021

Build failed (retrying...):

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Nov 11, 2021

bors r-

@craig
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <tobias.b.grieger@gmail.com>
Co-authored-by: Tobias Grieger <tobias.schottdorf@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbg tbg Awaiting requested review from tbg

1 more reviewer

@ajwerner ajwerner ajwerner approved these changes

Reviewers whose approvals may not affect merge requirements

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