sql,descs,*: rewrite by-ID and by-name descriptor lookups #94695
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
postamar
force-pushed
the
the-great-collection-api-refactor
branch
3 times, most recently
from
e06b2d6 to
9d19408
Compare
postamar
changed the title
postamar
force-pushed
the
the-great-collection-api-refactor
branch
2 times, most recently
from
a39452b to
71e68ee
Compare
postamar
changed the title
postamar
force-pushed
the
the-great-collection-api-refactor
branch
3 times, most recently
from
9bff432 to
1694cf6
Compare
postamar
requested review from
benbardin,
jayshrivastava and
cucaroach
and removed request for
a team
|
This is ready for review. The changes are mostly mechanical so there's not much point in going over this line-by-line but the changes in |
postamar
force-pushed
the
the-great-collection-api-refactor
branch
from
1694cf6 to
72b502e
Compare
|
Thanks for the review! |
postamar
force-pushed
the
the-great-collection-api-refactor
branch
from
72b502e to
77577b9
Compare
This commit replaces all Get(Mutable|Immutable)*By(ID|Name) methods on the descs.Collection and their usages with the recently-introduced By(ID|Name)Getter methods. While this commit shouldn't fundamentally change anything as far as functionality is concerned, it isn't strictly-speaking a refactor because the default behaviors have been changed slightly to make them consistent accross similar kinds of lookups. Here's an illustrative example: previously, GetImmutableDatabaseByID and GetImmutableSchemaByID would honor the Required flag while the other GetImmutable*ByID methods would ignore it and return an error when the descriptor is not found, presently all immutable by-ID lookups always return an error when the descriptor is not found. The new API does away with the tree.(Common|Object)LookupFlags and these have been cleaned up because now only the resolver uses them. Fixes cockroachdb#87753. Release note: None
postamar
force-pushed
the
the-great-collection-api-refactor
branch
from
77577b9 to
af6a72a
Compare
postamar
changed the title
|
bors r+ |
|
Build succeeded: |
kvoli
pushed a commit
to kvoli/cockroach
that referenced
this pull request
Jan 10, 2023
93755: streamingest: c2c UX fixes r=lidorcarmel a=lidorcarmel CREATE TENANT FROM REPLICATION: currently has an output (the job ids of the producer and consumer). This PR removes the output. ALTER TENANT PAUSE/RESUME: currently prints the job id, and this PR removes the output. ALTER TENANT COMPLETE: currently prints the job id. This PR changes the output to be the cutover timestamp. The rational is that the user may not know the cutover time because LATEST or NOW() etc. was used. Fixes: cockroachdb#94706 Epic: CRDB-18752 Release note: None 94695: sql,descs,*: rewrite by-ID and by-name descriptor lookups r=postamar a=postamar This commit replaces all Get(Mutable|Immutable)*By(ID|Name) methods on the descs.Collection and their usages with the recently-introduced By(ID|Name)Getter methods. While this commit shouldn't fundamentally change anything as far as functionality is concerned, it isn't strictly-speaking a refactor because the default behaviors have been changed slightly to make them consistent accross similar kinds of lookups. Here's an illustrative example: previously, GetImmutableDatabaseByID and GetImmutableSchemaByID would honor the Required flag while the other GetImmutable*ByID methods would ignore it and return an error when the descriptor is not found, presently all immutable by-ID lookups always return an error when the descriptor is not found. The new API does away with the tree.(Common|Object)LookupFlags and these have been cleaned up because now only the resolver uses them. Fixes cockroachdb#87753. Release note: None Co-authored-by: Lidor Carmel <[email protected]> Co-authored-by: Marius Posta <[email protected]>
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Mar 27, 2023
The check used for missing descriptors became incorrect in the course of cockroachdb#94695. That change updated the underlying error code used in getters by the GC job. The GC job would subsequently retry forever when the descriptor was missing. This bug has not been shipped yet, so not writing a release note. Fixes: cockroachdb#99590 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Mar 27, 2023
99458: jobs,*: stop writing payload and progress to system.jobs r=adityamaru a=adityamaru This change introduces a cluster version after which the payload and progress of a job will not be written to the system.jobs table. This will ensure that the system.job_info table is the single, source of truth for these two pieces of information. This cluster version has an associated upgrade that schema changes the `payload` column of the `system.jobs` table to be nullable, thereby allowing us to stop writing to it. This upgrade step is necessary for a future patch where we will drop the payload and progress columns. Without this intermediate upgrade step the `ALTER TABLE ... DROP COLUMN` upgrade job will attempt to write to dropped columns as part of its execution thereby failing to run the upgrade. Informs: #97762 Release note: None 99543: server: fix flaky drain test under race r=AlexTalks a=AlexTalks While previously `TestDrain` would issue a drain request twice, and expect that after the second drain request there would be no remaining leases, we have seen in some race builds that a lease extension can occur before that second drain, leaving one lease remaining after the second drain request. This can be seen in the following log example: ``` I230325 00:39:18.151604 14728 1@server/drain.go:145 ⋮ [T1,n1] 383 drain request received with doDrain = true, shutdown = false ... I230325 00:39:18.155547 986 kv/kvserver/replica_proposal.go:272 ⋮ [T1,n1,s1,r51/1:‹/Table/5{0-1}›,raft] 385 new range lease repl=(n1,s1):1 seq=1 start=0,0 exp=1679704764.152223164,0 pro=1679704758.152223164,0 following repl=(n1,s1):1 seq=1 start=0,0 exp=1679704746.135729956,0 pro=1679704740.135729956,0 I230325 00:39:18.172450 14728 1@server/drain.go:399 ⋮ [T1,n1] 386 (DEBUG) initiating kvserver node drain I230325 00:39:18.172613 14728 1@kv/kvserver/store.go:1559 ⋮ [T1,drain,n1,s1] 387 (DEBUG) store marked as draining I230325 00:39:18.182123 14728 1@server/drain.go:293 ⋮ [T1,n1] 388 drain remaining: 1 I230325 00:39:18.182249 14728 1@server/drain.go:295 ⋮ [T1,n1] 389 drain details: range lease iterations: 1 I230325 00:39:18.182404 14728 1@server/drain.go:175 ⋮ [T1,n1] 390 drain request completed without server shutdown ``` This change modifies the test to repeatedly issue drain requests until there is no remaining work, allowing the drain to complete upon subsequent requests. Fixes: #86974 Release note: None 99665: sql/gc_job,sqlerrors: make GC job robust to missing descriptors r=fqazi a=ajwerner ### sql: do not drop table descriptor independently if we're in drop schema If we have dropped schema IDs, we know that this is not an individual drop table schema change. We only have more than one dropped table when we drop a database or a schema. Before this change, we'd drop the table on its own, and then create another GC job to drop all the tables. This is not actually a bug because we should be robust to this, but it's also bad. ### sql/gc_job,sqlerrors: make GC job robust to missing descriptors The check used for missing descriptors became incorrect in the course of #94695. That change updated the underlying error code used in getters by the GC job. The GC job would subsequently retry forever when the descriptor was missing. This bug has not been shipped yet, so not writing a release note. Fixes: #99590 Release note (bug fix): DROP SCHEMA ... CASCADE could create multiple GC jobs: one for every table and one for the cascaded drop itself. This has been fixed. Co-authored-by: adityamaru <[email protected]> Co-authored-by: Alex Sarkesian <[email protected]> Co-authored-by: ajwerner <[email protected]>
blathers-crl bot
pushed a commit
that referenced
this pull request
Mar 27, 2023
The check used for missing descriptors became incorrect in the course of #94695. That change updated the underlying error code used in getters by the GC job. The GC job would subsequently retry forever when the descriptor was missing. This bug has not been shipped yet, so not writing a release note. Fixes: #99590 Release note: None
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this pull request
Mar 27, 2023
The check used for missing descriptors became incorrect in the course of cockroachdb#94695. That change updated the underlying error code used in getters by the GC job. The GC job would subsequently retry forever when the descriptor was missing. This bug has not been shipped yet, so not writing a release note. Fixes: cockroachdb#99590 Release note: None
rharding6373
pushed a commit
to rharding6373/cockroach
that referenced
this pull request
Mar 27, 2023
The check used for missing descriptors became incorrect in the course of cockroachdb#94695. That change updated the underlying error code used in getters by the GC job. The GC job would subsequently retry forever when the descriptor was missing. This bug has not been shipped yet, so not writing a release note. Fixes: cockroachdb#99590 Release note: None
aadityasondhi
pushed a commit
to aadityasondhi/cockroach
that referenced
this pull request
Mar 28, 2023
The check used for missing descriptors became incorrect in the course of cockroachdb#94695. That change updated the underlying error code used in getters by the GC job. The GC job would subsequently retry forever when the descriptor was missing. This bug has not been shipped yet, so not writing a release note. Fixes: cockroachdb#99590 Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit replaces all Get(Mutable|Immutable)*By(ID|Name) methods on the
descs.Collection and their usages with the recently-introduced
By(ID|Name)Getter methods.
While this commit shouldn't fundamentally change anything as far as
functionality is concerned, it isn't strictly-speaking a refactor
because the default behaviors have been changed slightly to make them
consistent accross similar kinds of lookups. Here's an illustrative
example: previously, GetImmutableDatabaseByID and GetImmutableSchemaByID
would honor the Required flag while the other GetImmutable*ByID methods
would ignore it and return an error when the descriptor is not found,
presently all immutable by-ID lookups always return an error when the
descriptor is not found.
The new API does away with the tree.(Common|Object)LookupFlags and these
have been cleaned up because now only the resolver uses them.
Fixes #87753.
Release note: None