kv, gossip: remove misc deprecated system config code by RichardJCai · Pull Request #82329 · cockroachdb/cockroach

RichardJCai · 2022-06-02T03:06:52Z

Release note: None

cockroach-teamcity · 2022-06-02T03:06:59Z

This change is

ajwerner

A thing of beauty! Let's get somebody on KV to have a look. Maybe @irfansharif or @nvanbenschoten?

ajwerner · 2022-06-02T03:20:26Z

pkg/testutils/localtestcluster/local_test_cluster.go

should we be deleting this?

I think we can delete this but I'll look into it into a follow up.

kvoli

LGTM, +@nvanbenschoten

nit: I think there should be a commit message. Referencing #76279 for posterity.

irfansharif

I've paged a lot of this out, surprisingly, but the broad strokes LGTM. I'm curious what's left in order to get rid of all this code:

cockroach/pkg/config/system.go

Lines 76 to 84 in 609825e

    
           // SystemConfig embeds a SystemConfigEntries message which contains an 
        
           // entry for every system descriptor (e.g. databases, tables, zone 
        
           // configs). It also has a map from object ID to unmarshaled zone 
        
           // config for caching. 
        
           // The shouldSplitCache caches information about the descriptor ID, 
        
           // saying whether or not it should be considered for splitting at all. 
        
           // A database descriptor or a table view descriptor are examples of IDs 
        
           // that should not be considered for splits. 
        
           type SystemConfig struct {

.

pkg/kv/kvclient/kvcoord/txn_interceptor_committer_test.go

pkg/kv/kvserver/queue_test.go

pkg/kv/txn.go

irfansharif · 2022-06-03T16:13:57Z

pkg/kv/kvserver/store.go

@@ -2193,7 +2193,6 @@ func (s *Store) GetConfReader(ctx context.Context) (spanconfig.StoreReader, erro

 	if s.cfg.SpanConfigsDisabled ||


Should we get rid of all these knobs now that we're no longer gossipping the system config span: SpanConfigsDisabled, spanconfigstore.EnabledSetting, s.TestingKnobs().UseSystemConfigSpanForQueues.

Did you mean to resolve this comment? If we want to get rid of these in future commits, worth leaving a TODO.

I swear I replied but the message disappeared.

I tried removing it but some tests were failing and it wasn't obvious to me why. I added two more TODOs to remove SpanConfigsDisabled and spanconfigstore.EnabledSetting and you already had one for s.TestingKnobs().UseSystemConfigSpanForQueues

pkg/testutils/localtestcluster/local_test_cluster.go

RichardJCai · 2022-06-06T20:00:59Z

@irfansharif do you want to take another look here? I have some follow work here as well
#82281

irfansharif

or at least update SystemConfigSpan to contain just system.descriptor and
system.zones.

I'm not sure why we need an unsplittable span to contain those two tables specifically but happy to learn in your subsequent PRs. This is really great stuff, thanks for cleaning up the mess.

irfansharif · 2022-06-07T18:55:56Z

pkg/kv/kvclient/kvcoord/txn_interceptor_committer_test.go

-			SystemConfigSpan: true,
-		},
-	}
+	etArgsWithTrigger.InternalCommitTrigger = &roachpb.InternalCommitTrigger{}


We don't need this line at all, right?

We still do, it changes how the lock span is populated below.
I'm just gonna update it to use a NodeLiveness span instead in the Trigger.

irfansharif · 2022-06-07T18:58:43Z

pkg/kv/kvserver/store.go

@@ -2193,7 +2193,6 @@ func (s *Store) GetConfReader(ctx context.Context) (spanconfig.StoreReader, erro

 	if s.cfg.SpanConfigsDisabled ||


Did you mean to resolve this comment? If we want to get rid of these in future commits, worth leaving a TODO.

RichardJCai · 2022-06-07T20:13:12Z

I've paged a lot of this out, surprisingly, but the broad strokes LGTM. I'm curious what's left in order to get rid of all this code:

Added a comment for this.

// NB: SystemConfig can be updated to only contain system.descriptor and
// system.zones. We still need SystemConfig for SystemConfigProvider which is
// used in replication reports and the opt catalog.

There is still some work left to actually remove `SystemConfigSpan` or at least update `SystemConfigSpan` to contain just system.descriptor and system.zones. cockroachdb#76279 Release note: None

RichardJCai · 2022-06-07T21:44:56Z

Thanks for reviewing!

bors r+

craig · 2022-06-07T23:46:36Z

Build succeeded:

GitHub CI (Cockroach)

140160: sql: rollback transaction eagerly upon error r=rafiss a=rafiss ### sql: rollback transaction eagerly upon error If we have not used any savepoints, it's safe to rollback the transaction immediately when a non-retryable error is encountered. This releases locks sooner, which is useful so we avoid needing to wait until the client sends a ROLLBACK statement. Release note (bug fix): Transactions that enter the aborted state will now release locks they are holding immediately, as long as there is no SAVEPOINT active in the transaction. ### sql: make transaction_timeout abort transaction eagerly Previously, the transaction_timeout setting would only cause the transaction to abort if the timeout had already expired by the time the next statement is received by CRDB. Now we create a timer that fires if the timeout expires while we are waiting for the next statement. There's no release note, since up until the previous commit, which changed the behavior so transactions eagerly release locks, there was no functional difference caused by lazily timing out the transaction. Release note: None ### sql: add test for implicit txn holding locks across retries Release note: None --- fixes #140234 141815: systemconfigwatcher: remove NewWithAdditionalProvider r=rafiss a=rafiss This was needed in the 21.2-22.1 mixed version state, before tenants had a spanconfig reconciliation job. Now that the job exists, there's no need to pass in an additional provider for tenants. This takes the work in #82329 a bit further. informs #76625 Release note: None Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>

ajwerner reviewed Jun 2, 2022

View reviewed changes

RichardJCai force-pushed the remove_keys_system_span_config branch from 12ac40d to 21ddc67 Compare June 2, 2022 14:52

RichardJCai marked this pull request as ready for review June 2, 2022 14:52

RichardJCai requested review from a team as code owners June 2, 2022 14:52

RichardJCai requested a review from a team June 2, 2022 14:52

RichardJCai requested a review from a team as a code owner June 2, 2022 14:52

RichardJCai requested review from irfansharif and nvb June 2, 2022 15:12

kvoli approved these changes Jun 3, 2022

View reviewed changes

kvoli assigned nvb Jun 3, 2022

irfansharif reviewed Jun 3, 2022

View reviewed changes

RichardJCai force-pushed the remove_keys_system_span_config branch from 21ddc67 to c122f24 Compare June 3, 2022 21:18

blathers-crl bot requested a review from irfansharif June 3, 2022 21:18

RichardJCai force-pushed the remove_keys_system_span_config branch from c122f24 to 4b1be2e Compare June 3, 2022 21:41

irfansharif approved these changes Jun 7, 2022

View reviewed changes

RichardJCai changed the title ~~kv, gossip: remove deprecated system span config~~ kv, gossip: remove misc deprecated system config code Jun 7, 2022

kv, gossip: remove misc deprecated system config code

dc80d3c

There is still some work left to actually remove `SystemConfigSpan` or at least update `SystemConfigSpan` to contain just system.descriptor and system.zones. cockroachdb#76279 Release note: None

RichardJCai force-pushed the remove_keys_system_span_config branch from 4b1be2e to dc80d3c Compare June 7, 2022 20:17

craig bot merged commit 0c8f826 into cockroachdb:master Jun 7, 2022

This was referenced Feb 21, 2025

gossip,kvserver,sql: remove the last vestiges of the systemconfig gossip in 22.2. #76625

Closed

systemconfigwatcher: remove NewWithAdditionalProvider #141815

Merged

	// SystemConfig embeds a SystemConfigEntries message which contains an
	// entry for every system descriptor (e.g. databases, tables, zone
	// configs). It also has a map from object ID to unmarshaled zone
	// config for caching.
	// The shouldSplitCache caches information about the descriptor ID,
	// saying whether or not it should be considered for splitting at all.
	// A database descriptor or a table view descriptor are examples of IDs
	// that should not be considered for splits.
	type SystemConfig struct {

		@@ -2193,7 +2193,6 @@ func (s *Store) GetConfReader(ctx context.Context) (spanconfig.StoreReader, erro

		if s.cfg.SpanConfigsDisabled \|\|

Conversation

RichardJCai commented Jun 2, 2022

Uh oh!

cockroach-teamcity commented Jun 2, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvoli left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RichardJCai commented Jun 6, 2022

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardJCai commented Jun 7, 2022

Uh oh!

RichardJCai commented Jun 7, 2022

Uh oh!

craig bot commented Jun 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants