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

schema: lose gossiped system config dep in schemaChangeGCResumer #49691

Closed
tbg opened this issue May 29, 2020 · 2 comments
Closed

schema: lose gossiped system config dep in schemaChangeGCResumer #49691

tbg opened this issue May 29, 2020 · 2 comments
Assignees
@nvanbenschoten
Labels
A-multitenancy Related to multi-tenancy C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.

Comments

@tbg
Copy link
Member

tbg commented May 29, 2020

@ajwerner you probably have this tracked somewhere more generally in avoiding the system config gossip altogether, if so please close this issue but make sure you have A-multitenancy-blocker on the other one. Thanks!
@tbg tbg added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-multitenancy-blocker labels May 29, 2020
@tbg tbg mentioned this issue May 29, 2020
Merged
tbg added a commit to tbg/cockroach that referenced this issue May 29, 2020
@tbg
server: use "read-only" Gossip for tenants
e487d70 
We were previously using the Gossip instance of the TestServer against
which the tenant was initialized.

This commit trims the dependency further by initializing its own Gossip
instance which is never written to (i.e. `AddInfo` is never called) and
which does not accept incoming connections.

As a reminder, the remaining problematic uses of Gossip as of this
commit are:

- making a `nodeDialer` (for `DistSender`), tracked in:
  cockroachdb#47909
- access to the system config:
  - `(schemaChangeGCResumer).Resume`, tracked:
    cockroachdb#49691
  - `database.Cache`, tracked:
    cockroachdb#49692
- `(physicalplan).spanResolver` (for replica oracle).
  This is likely not a blocker as we can use a "dumber" oracle in this case;
  the oracle is used for distsql physical planning of which tenants will
  do none. Tracked in: cockroachdb#48432

Release note: None
tbg added a commit to tbg/cockroach that referenced this issue Jun 2, 2020
@tbg
server: use "read-only" Gossip for tenants
b8b78a4 
We were previously using the Gossip instance of the TestServer against
which the tenant was initialized.

This commit trims the dependency further by initializing its own Gossip
instance which is never written to (i.e. `AddInfo` is never called) and
which does not accept incoming connections.

As a reminder, the remaining problematic uses of Gossip as of this
commit are:

- making a `nodeDialer` (for `DistSender`), tracked in:
  cockroachdb#47909
- access to the system config:
  - `(schemaChangeGCResumer).Resume`, tracked:
    cockroachdb#49691
  - `database.Cache`, tracked:
    cockroachdb#49692
- `(physicalplan).spanResolver` (for replica oracle).
  This is likely not a blocker as we can use a "dumber" oracle in this case;
  the oracle is used for distsql physical planning of which tenants will
  do none. Tracked in: cockroachdb#48432

Release note: None
tbg added a commit to tbg/cockroach that referenced this issue Jun 2, 2020
@tbg
server: use "read-only" Gossip for tenants
4d036c3 
We were previously using the Gossip instance of the TestServer against
which the tenant was initialized.

This commit trims the dependency further by initializing its own Gossip
instance which is never written to (i.e. `AddInfo` is never called) and
which does not accept incoming connections.

As a reminder, the remaining problematic uses of Gossip as of this
commit are:

- making a `nodeDialer` (for `DistSender`), tracked in:
  cockroachdb#47909
- access to the system config:
  - `(schemaChangeGCResumer).Resume`, tracked:
    cockroachdb#49691
  - `database.Cache`, tracked:
    cockroachdb#49692
- `(physicalplan).spanResolver` (for replica oracle).
  This is likely not a blocker as we can use a "dumber" oracle in this case;
  the oracle is used for distsql physical planning of which tenants will
  do none. Tracked in: cockroachdb#48432

Release note: None
craig bot pushed a commit that referenced this issue Jun 2, 2020
@tbg @yuzefovich
Merge #49693 #49724
3b2245f 
49693: server: use "read-only" Gossip for tenants r=asubiotto,nvanbenschoten a=tbg

We were previously using the Gossip instance of the TestServer against
which the tenant was initialized.

This commit trims the dependency further by initializing its own Gossip
instance which is never written to (i.e. `AddInfo` is never called) and
which does not accept incoming connections.

As a reminder, the remaining problematic uses of Gossip as of this
commit are:

- making a `nodeDialer` (for `DistSender`), tracked in:
  #47909
- access to the system config:
  - `(schemaChangeGCResumer).Resume`, tracked:
    #49691
  - `database.Cache`, tracked:
    #49692
- `(physicalplan).spanResolver` (for replica oracle).
  This is likely not a blocker as we can use a "dumber" oracle in this case;
  the oracle is used for distsql physical planning of which tenants will
  do none. Tracked in: #48432

cc @ajwerner 

Release note: None

49724: sql: clean up of scanNode and some other things r=yuzefovich a=yuzefovich

**sql: unify PlanningCtx constructors into one**

Release note: None

**sql: remove separate scanVisilibity struct**

This commit removes `sql.scanVisibility` in favor of protobuf-generated
`execinfrapb.ScanVisibility`. It also introduces prettier aliases for
the two values into `execinfra` package that are now used throughout the
code.

Release note: None

**sql: clean up of scan node and a few other things**

This commit does the following cleanups of `scanNode`:
1. refactors `scanNode.initCols` method to be standalone (it will
probably be reused later by distsql spec exec factory).
2. removes `numBackfillColumns`, `specifiedIndexReverse`, and
`isSecondaryIndex` fields since they are no longer used.
3. refactors the code to remove `valNeededForCols` field which was
always consecutive numbers in range `[0, len(n.cols)-1]`.
4. refactors `getIndexIdx` method to not depend on `scanNode`.

Additionally, this commit removes `planDependencies` business
from `planTop` since optimizer now handles CREATE VIEW and handles
the plan dependencies on its own (and CREATE VIEW was the single
user of that struct in the plan top).

Also, it removes (which seems like) unnecessary call to `planColumns`
when creating distinct spec and an unused parameter from
`createTableReaders` method.

Addresses: #47474.

Release note: None

Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
@ajwerner
Copy link
Contributor

ajwerner commented Jul 9, 2020

This in many ways duplicates #49445.

@tbg tbg assigned nvanbenschoten and unassigned ajwerner Jul 15, 2020
@nvanbenschoten
Copy link
Member

This in many ways duplicates #49445.

Yes, it's a duplicate. I'm going to close this one to avoid the noise.

@tbg tbg added the A-multitenancy Related to multi-tenancy label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nvanbenschoten nvanbenschoten

Labels
A-multitenancy Related to multi-tenancy C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ajwerner @tbg @nvanbenschoten