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: split the tenant server creation into 3 stages #90384

Merged
merged 22 commits into from
Oct 26, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Oct 20, 2022

Needed as prerequisite to #90176, towards fixing #89974.
First two commits from #90523.

The original motivation (and ultimate goal) for this PR is to
split the tenant server initialization into three phases: New(),
PreStart(), AcceptClients(), so as to reuse a common process startup
logic in a separate PR (#90176).

To achieve this goal, this PR re-orders the initialization steps
in server.NewTenantServer (previously known as
server.StartTenant), and extracts many of them into a new method
(*SQLServerWrapper).PreStart().

The specific order of the code in NewTenantServer() and
(*SQLServerWrapper).PreStart() was chosen to mirror the order of
things in NewServer() and (*Server).PreStart().

Reason for using the same order:

  • it makes the review of this change easier: the reviewer can pull
    server.go and tenant.go and read them side-by-side, to satisfy
    themselves that the two implementations of
    NewServer/NewTenantServer and PreStart are equivalent.

  • it will make it easier for future maintainers to keep them in sync.

  • it helps us discover the fact that both sides share a lot of code.
    This opens an opportunity to merge them to a common implementation
    at a later stage.

While doing this work, care was also taken to discover which steps of
(*Server).PreStart() were missing from the tenant server
initialization. We found the following:

  • the Sentry context enhancement (to report cluster ID, etc)
    was missing. This commit fixes that.

  • several log entries that report the server configuration
    to the OPS channel were not emitted. This commit fixes that.

  • the Graphite metric reporting was never enabled, even when
    configured. This commit fixes that.

  • the Obs Service testing knobs (TestingKnobs.EventExporter) were
    not configured on the ObsServer instance. This commit fixes that.

  • the go.scheduler_latency metric was not being measured.
    This commit fixes that.

Additionally, two followup issues were filed for the following
missing steps:

Epic: CRDB-14537

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20221020-split-tenant-init branch 4 times, most recently from 91e9deb to 0b969a6 Compare October 23, 2022 19:32
@knz knz marked this pull request as ready for review October 23, 2022 19:33
@knz knz requested review from a team as code owners October 23, 2022 19:33
@knz knz requested a review from a team October 23, 2022 19:33
@knz knz requested review from a team as code owners October 23, 2022 19:33
@knz knz requested review from herkolategan and smg260 and removed request for a team October 23, 2022 19:33
@knz
Copy link
Contributor Author

knz commented Oct 23, 2022

@jeffswenson @dhartunian I'd like to invite you both to review this, with different focus:

  • @jeffswenson to satisfy yourself that the changes here are feature-neutral, despite the relative large code movement. (Hint: look at the suggestion in the PR description to place the source files side-by-side to make review easier.)
  • @dhartunian because we have discussed this area further, and I know you have an interest in keeping your understanding of this area up-to-date.

cc @andreimatei.

@knz knz requested review from dhartunian and jeffswenson October 23, 2022 19:34
craig bot pushed a commit that referenced this pull request Oct 25, 2022
90523: server: start purging web_sessions in secondary tenants r=aadityasondhi,stevendanna a=knz

Fixes #90522.

The next PR #90384 will ensure this code lands in the right place.


90595: catalog/descs: fix memory allocation when resolving many descriptors r=ajwerner a=ajwerner

In some cases we resolve many descriptors and the fast int map is a bad fit. We just really wanted to optimize the N=1 case.

Epic: CRDB-20865

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
@knz knz force-pushed the 20221020-split-tenant-init branch from a4a0b98 to 637f28c Compare October 26, 2022 18:12
knz added 20 commits October 26, 2022 21:47
The `go.scheduler_latency` metric was not being measured.
This commit fixes that.

Release note: None
The Graphite metric reporting was never enabled, even when
configured. This commit fixes that.

Release note: None
The Sentry context enhancement (to report cluster ID, etc)
was missing. This commit fixes that.

Release note: None
The original motivation (and ultimate goal) for this commit is to
split the tenant server initialization into three phases: `New()`,
`PreStart()`, `AcceptClients()`, so as to reuse a common process startup
logic in a separate (later) commit.

To achieve this goal, this commit re-orders the initialization steps
in `server.NewTenantServer` (previously known as
`server.StartTenant`), and extracts many of them into a new method
`(*SQLServerWrapper).PreStart()`.

The specific order of the code in `NewTenantServer()` and
`(*SQLServerWrapper).PreStart()` was chosen to mirror the order of
things in `NewServer()` and `(*Server).PreStart()`.

Reasons for using the same order:

- it makes the review of this change easier: the reviewer can pull
  `server.go` and `tenant.go` and read them side-by-side, to satisfy
  themselves that the two implementations of
  `NewServer`/`NewTenantServer` and `PreStart` are equivalent.

- it will make it easier for future maintainers to keep them in sync.

- it helps us discover the fact that both sides share a lot of code.
  This opens an opportunity to merge them to a common implementation
  at a later stage.

While doing this work, care was also taken to discover which steps of
`(*Server).PreStart()` were *missing* from the tenant server
initialization. We found the following:

- the Sentry context enhancement (to report cluster ID, etc)
  was missing. This commit fixes that.

- several log entries that report the server configuration
  to the OPS channel were not emitted. This commit fixes that.

- the Graphite metric reporting was never enabled, even when
  configured. This commit fixes that.

- the Obs Service testing knobs (TestingKnobs.EventExporter) were
  not configured on the ObsServer instance. This commit fixes that.

- the `go.scheduler_latency` metric was not being measured.
  This commit fixes that.

Additionally, two followup issues were filed for the following
missing steps:

- missing support for the special file that blocks background jobs. (cockroachdb#90524)
- missing support for the system.eventlog cleanup loop. (cockroachdb#90521)

Release note: None
@knz knz force-pushed the 20221020-split-tenant-init branch from 637f28c to 616c822 Compare October 26, 2022 19:48
@knz
Copy link
Contributor Author

knz commented Oct 26, 2022

TFYR

bors r=dhartunian

@craig
Copy link
Contributor

craig bot commented Oct 26, 2022

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