Skip to content

release-22.2: sql/sqlutil, descs: move TxnWithExecutor() and methods to sql.InternalExecutorFactory#89940

Merged
ZhouXing19 merged 3 commits into
cockroachdb:release-22.2from
ZhouXing19:backport22.2-move-ie
Oct 14, 2022
Merged

release-22.2: sql/sqlutil, descs: move TxnWithExecutor() and methods to sql.InternalExecutorFactory#89940
ZhouXing19 merged 3 commits into
cockroachdb:release-22.2from
ZhouXing19:backport22.2-move-ie

Conversation

@ZhouXing19
Copy link
Copy Markdown
Collaborator

@ZhouXing19 ZhouXing19 commented Oct 13, 2022 •
edited
Loading

Backport 3/3 commits from #88875.

/cc https://github.com/orgs/cockroachdb/teams/release

Having TxnWithExecutor() in descs.CollectionFactory is unnatural, and will bring dependency loop headaches. This commit is to move the same logic under sql.InternalExecutorFactory.

link Epic CRDB-19135

Release note: None

Release justification: Bug fixes and low-risk updates to usages of internal executor.

@ZhouXing19
sql: add TxnWithExecutor() and methods to InternalExecutorFactory
2c29700 
Having `TxnWithExecutor()` in `descs.CollectionFactory` is unnatural, and will
bring dependency loop headaches. This commit is to add the same logic under
`InternalExecutorFactory`. We will remove
`descs.CollectionFactory.TxnWithExecutor()` in a future PR.

Release note: None
@ZhouXing19
*: replacing descs.CollectionFactory with descs.TxnManager
128d009 
This is part of commit for a mechanical move of the `TxnWithExecutor()` function
from the `descs` pkg to the `sql` package. We now use the `descs.TxnManager`
interface for the same logic.

Release note: None
@ZhouXing19 ZhouXing19 requested a review from a team October 13, 2022 20:54
@ZhouXing19 ZhouXing19 requested review from a team as code owners October 13, 2022 20:54
@ZhouXing19 ZhouXing19 requested a review from a team October 13, 2022 20:54
@ZhouXing19 ZhouXing19 requested review from a team as code owners October 13, 2022 20:54
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Oct 13, 2022

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@ZhouXing19 ZhouXing19 requested review from rhu713, shermanCRL and srosenberg and removed request for a team October 13, 2022 20:54
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Oct 13, 2022

This change is Reviewable

@ZhouXing19 ZhouXing19 mentioned this pull request Oct 13, 2022
Merged
@rafiss
Copy link
Copy Markdown
Collaborator

rafiss commented Oct 13, 2022

hm, why did it not backport without conflicts? also i see a lint failure:

the following objects are unused: github.com/cockroachdb/cockroach/pkg/upgrade/upgrademanager.cf

@ZhouXing19
*: replace descs.CollectionFactory.Txn() and .TxnWithExecutor() w…
6196213 
…ith new interfaces

We now use `descs.TxnManager.DescsWithTxn()` and
`.DescsTxnWithExecutor()` to replace `descs.CollectionFactory.Txn()`
and `.TxnWithExecutor()`.

Release note: None
@ZhouXing19 ZhouXing19 force-pushed the backport22.2-move-ie branch from 0d4356e to 6196213 Compare October 13, 2022 22:44
@ZhouXing19
Copy link
Copy Markdown
Collaborator Author

ZhouXing19 commented Oct 13, 2022 •
edited
Loading

hm, why did it not backport without conflicts?

Because I backport this too late 😢 Another PR that depends on this PR has been backported, so I have to adjust for that PR too to use my changes Oh for the automatic backport when the original PR got merged? Not sure why there were conflicts... When I made this PR, the only conflict I encountered is from pkg/server/status.go

@ZhouXing19 ZhouXing19 merged commit 7a5c255 into cockroachdb:release-22.2 Oct 14, 2022
@ZhouXing19 ZhouXing19 mentioned this pull request Oct 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rafiss rafiss rafiss approved these changes

@rhu713 rhu713 Awaiting requested review from rhu713 rhu713 is a code owner automatically assigned from cockroachdb/disaster-recovery

@shermanCRL shermanCRL Awaiting requested review from shermanCRL shermanCRL is a code owner automatically assigned from cockroachdb/cdc-prs

@srosenberg srosenberg Awaiting requested review from srosenberg srosenberg is a code owner automatically assigned from cockroachdb/test-eng

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ZhouXing19 @cockroach-teamcity @rafiss