-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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.
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
*: milisecond test and infoschema fix #24820
Conversation
// infoschema maybe null for tests | ||
if is, ok := e.ctx.GetSessionVars().GetInfoSchema().(infoschema.InfoSchema); ok { | ||
reqBuilder.SetFromInfoSchema(is) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part of logic is not the same as before? nil
interface should be handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought GetInfoSchema
will always return something. I did not expect that domain.InfoSchema()
will return nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Now request builder will check it by is, ok := emptyInterce.(InfoSchema)
.
/run-build |
/run-all-tests |
OK, a mistake. Looks like |
/run-build |
OK.. Got another panic and fixed. So planner relies on that snapshot infoschema for testing, and I must provide a partial mock.. |
/run-all-tests |
1 similar comment
/run-all-tests |
/run-integration-common-test |
/run-all-tests |
Why trigger all tests again and again? From the log, the error is reported by |
/run-all-tests tidb-test=pr/1195 |
CI just upgrade go version from 1.13 to 1.16.4. Here we want to check again |
/run-sqllogic-test-1 tidb-test=pr/1195 |
1 similar comment
/run-sqllogic-test-1 tidb-test=pr/1195 |
/run-integration-common-test |
1 similar comment
/run-integration-common-test |
/lgtm |
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
/merge |
@nolouch: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 2c131aa
|
// So we give a convenience for that. | ||
// FIXME: remove this interface | ||
type InfoschemaMetaVersion interface { | ||
SchemaMetaVersion() int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For interface used as a tool to avoid circular, we can put it in the util package.
But for this, it's ok.
/lgtm |
/run-all-tests |
/merge |
@xhebox: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: close #24812
Problem Summary: Failed because of time prec. While fixing the problem, I found that when
commit
is executed,TxnCtx.InfoSchema
is not refreshed, it is lazily refreshed. ThensessionVars.GetInfoSchema()
will return the old infoschema aftercommit
.Later commits will hopefully remove the usage of
dom.InfoSchema()
and refineTxnCtx
. But for now, let us merge this PR first.What is changed and how it works?
What's Changed:
vars.TxnCtx != nil && vars.TxnCtx.IsExplicit
forGetInfoSchema()
. Now it will not return the old infoschema aftercommit
, but will still return nil.domain.GetDomain(sctx).InfoSchema()
forGetInfoSchema()
to return the latest commit aftercommit
. But there is a circular dependency loop, it is moved from a getter ofsessionVars
to a method ofsessionctx.Context
.builder.Request.SchemaVar = is.SchemaMetaVersion()
inSetFromSessionVars
and addbuilder.Request.SchemaVar = is.SchemaMetaVersion()
inSetFromInfoSchema
. SeveralSetFromInfoSchema
was added to ensure infoschema is set correctly.Check List
Tests
Release note