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,*: remove remaining uses of OptionalErr, UnsupportedWithMultiTenancy, OptionalGossip and related code #100826

Open
knz opened this issue Apr 6, 2023 · 2 comments
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-db-server

Comments

@knz
Copy link
Contributor

knz commented Apr 6, 2023

Describe the problem

The code still currently contains "imperfect" code paths that ought to be either generalized (to become independent of whether they run in system tenant or secondary tenant); or be entirely specialized to the system tenant because they don't make sense in secondary tenants at all (for example if they pertain exclusively to operating the storage layer).

Eventually we want all the SQL layer to not use KV-level data structures directly. All should go through a KV API over RPC and checked via the capability subsystem.

This issue is a dependency of #54252.

To Reproduce

Search for the following:

  • OptionalErr
  • OptionalGossip
  • OptionalNodeID
  • OptionalNodesStatusServer
  • TenantSQLDeprecatedWrapper
  • UnsupportedWithMultiTenancy

All these need to be eliminated.

Jira issue: CRDB-26645

Epic CRDB-26091

@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 6, 2023
@exalate-issue-sync exalate-issue-sync bot added the T-multitenant Issues owned by the multi-tenant virtual team label Apr 7, 2023
@exalate-issue-sync exalate-issue-sync bot changed the title server,*: remove remaining uses of OptionalErr, UnsupportedWithMultiTenancy and related code server,*: remove remaining uses of OptionalErr, UnsupportedWithMultiTenancy, OptionalGossip and related code Apr 7, 2023
craig bot pushed a commit that referenced this issue Jun 14, 2023
104820: backupccl: adjust a test to run for secondary tenant codec too r=yuzefovich a=yuzefovich

Fixes: #82882.

Release note: None

104823: row: remove leftover reference to TestingSQLCodec in a test r=yuzefovich a=yuzefovich

This was missed in #82890.

Addresses: #48123.
Epic: None

Release note: None

104828: base: remove OptionalNodeIDErr r=yuzefovich a=yuzefovich

This method was only used in one place where we need to get the SQL instance ID of the gateway. That place has been refactored to pass that ID explicitly from the DistSQLPlanner.

Addresses: #100826.
Epic: None

Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Jun 14, 2023
104089: testutils: add fingerprint utility package r=stevendanna a=msbutler

This patch adds a couple helper methods that make it easier to: fingerprint a table, database or cluster, compare fingerprints, and identify a mismatch on the table level. All fingerprinting methods call crdb_internal.fingerprint() on the table span level with the user provided options.

This patch also integrates the new methods in the backupccl and streamingccl codebases.

A future patch could add a helper method that identifies mismatched keys within a mismatched table.

Informs #103072

Release note: None

104828: base: remove OptionalNodeIDErr r=yuzefovich a=yuzefovich

This method was only used in one place where we need to get the SQL instance ID of the gateway. That place has been refactored to pass that ID explicitly from the DistSQLPlanner.

Addresses: #100826.
Epic: None

Release note: None

104888: kvserver: clarify `kv.raft_log.disable_synchronization_unsafe` r=erikgrinaker a=erikgrinaker

This setting not only disables fsync, it also disables flushing writes to the OS, so it will lose data even on process crashes.

Epic: none
Release note: None

Co-authored-by: Michael Butler <butler@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com>
craig bot pushed a commit that referenced this issue Jul 11, 2023
106326: server: unify and harden crdb_internal.cluster_inflight_traces r=yuzefovich a=yuzefovich

This commit unifies population of `crdb_internal.cluster_inflight_traces` between single-tenant and multi-tenant modes - from now on, in all deployment modes we use all available SQL instances to fetch span recordings from. The trace collector has been refactored accordingly.

Additionally, the trace collector is hardened to ignore any errors that are encountered when retrieving recordings from a single SQL instance (previously, it would fail if we encountered an error when dialing the SQL instance or when receiving GetSpanRecordingResponse, and now we will log a warning and proceed silently). The rationale for making this change is that instances might no longer be "ready" when the iteration reaches them, so this should make the virtual table generation more bullet-proof.

Epic: CRDB-26691
Touches: #100826

Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@stevendanna stevendanna added T-db-server and removed T-multitenant Issues owned by the multi-tenant virtual team labels Jul 9, 2024
@stevendanna
Copy link
Collaborator

@shubhamdhama This one might be interesting to take a peak at and see how much work is left here. We might be pretty close to having this done.

@shubhamdhama
Copy link
Contributor

We discussed to split this issue into smaller tasks and deal with them in separate PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-db-server
Projects
None yet
Development

No branches or pull requests

3 participants