spanconfigsqltranslator: add sqlutil.InternalExecutor to SQLTranslator #91401
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ZhouXing19)
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go line 46 at r1 (raw file):
txn *kv.Txn descsCol *descs.Collection ie sqlutil.InternalExecutor
nit: could you add a TODO here about its intended use if you're not planning on making use of it in this patch?
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go line 72 at r1 (raw file):
// NewSQLTranslator constructs and returns a transaction-scoped // spanconfig.SQLTranslator. The caller must ensure that the collection passed // in is associated with the supplied transaction.
nit: Could you update this comment to now reference the internal executor as well? Something like "The caller must ensure that the passed in collection, internal executor, and transaction are associated with each other"?
ZhouXing19
force-pushed
the
add-ie-sqltranslator
branch
from
e103c71
to
793656e
Compare
There was a problem hiding this comment.
Thanks @arulajmani for reviewing! I made a txnBundle struct to emphasize that the descriptor collection, internal execuot and the txn should be associated with each other. Do you think this may be better?
Reviewable status:
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go line 46 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: could you add a TODO here about its intended use if you're not planning on making use of it in this patch?
Done.
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go line 72 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: Could you update this comment to now reference the internal executor as well? Something like "The caller must ensure that the passed in collection, internal executor, and transaction are associated with each other"?
Done.
This commit is part of the effort of having an internal executor better bound to its outer txn if there's one. The next step after this commit is to replace the executor used in `s.ptsProvider.GetState()` in `SQLTranslator.Translate()` to the one hanging off `SQLTranslator`. Release note: None
ZhouXing19
force-pushed
the
add-ie-sqltranslator
branch
from
793656e
to
3d80db3
Compare
|
The added change is pretty small so I'll just merge it. |
|
Build failed (retrying...): |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build succeeded: |
|
Sorry, I just saw this notification. I'm all for the |
This commit is part of the effort of having an internal executor better bound to its outer txn if there's one.
The next step after this commit is to replace the executor used in
s.ptsProvider.GetState()inSQLTranslator.Translate()to the one hanging offSQLTranslator.Informs: #91004
Release note: None