*: modify GetGlobalConfig() to provide a thread safe API for unit test #17954
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
tiancaiamao
added
type/enhancement
github-actions
bot
added
sig/sql-infra
lysu
reviewed
Jun 11, 2020
| @@ -139,13 +139,17 @@ func (tk *TestKit) GetConnectionID() { | |||
|
|
|||
| // Exec executes a sql statement. | |||
| func (tk *TestKit) Exec(sql string, args ...interface{}) (sqlexec.RecordSet, error) { | |||
| return tk.ExecWithCtx(context.Background(), sql, args) | |||
There was a problem hiding this comment.
Suggested change
| return tk.ExecWithCtx(context.Background(), sql, args) | |
| return tk.ExecWithCtx(context.Background(), sql, args...) |
|
This change has performance cost, we have to lookup for the context every time we need the global config. This PR fix the race problem in another way #17964 |
|
I think this PR provides a test-local config mechanism, which allows a few tests in |
|
@tangenta Making an isolated env to run test requires much more effort. |
|
#17964 tried another way to fix DATA RACE |
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.
What problem does this PR solve?
Problem Summary:
There is only one instance of global config, providing that many test cases run concurrently,
when an unit test modify the global config, it is easy to DATA RACE.
We have been struggle with this problem, in this commit I modify the
GetGlobalConfigAPI to provide a thread safe mechanism for each test to use its own config.What is changed and how it works?
What's Changed:
GetGlobalConfig()toGetGlobalConfig(ctx)config.WithGlobalConfig()functionHow it Works:
GetGlobalConfig will check the context first, if it find the
overrideConfigForTestfrom the context, use that.Each test can use
MustExecWithCtx()to override the global config for testing.Check List
Tests
Side effects
Modify exposed API break code compatibility
Release note