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.
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
infoschema, executor, txn: implement DATA_LOCK_WAITS table #24750
infoschema, executor, txn: implement DATA_LOCK_WAITS table #24750
Changes from all commits
0967709
f8fc3ae
bb368cf
85a2e28
afdb321
0824c0a
a30dba1
420b5ea
2d29800
6aeea7c
96c0d42
bcc7a5c
36df5e1
c869d79
e9b23ab
a3aebf0
38537e1
eb7c354
5d60603
1f6f44d
ee154c1
e72acea
32adcb9
ce1c7bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It seems you also need to implement a TearDownSuite method to close these resources.
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 the more proper way to write the unit tests is to implement the GetLockWaitInfo api for the unistore, and construct a real lock waiting here. Since finishing that kind of tests would be too complicated and we may not have so much time, it's ok to me that we do not do that in this PR. But I think the tests need to be improved in the future.
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 there should be a unit test on unistore for testing GetLockWaitInfo api. (Will implement in the future)
And an itegration test for the whole DATA_LOCK_WAIT stuff for the whole system of TiDB-TiKV/unistore.(But I'm not very sure where should I put that test in.)
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.
You can see my tests for the deadlocks table, perhaps it may help you. If some integration tests are hard to implement in TiDB repo, or you relies on some specific configuration of the cluster, you can do it in UTF.
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.
Can you reuse the
SetUpSuite
andTearDownSuite
of thetestTableSuiteBase
, which means, invokings.testTableSuiteBase.SetUpSuite
and only do additional work in your own code?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.
SetUpSuite is very different with testTableSuiteBase, but we can just use testTableSuiteBase's TearDownSuite here. So no need to implement TearDownSuite.
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.
If they are totally different, I think it's better to not nesting the testTableSuiteBase here, but writing the similar fields by yourself. But let's merge the PR first and you can do code refinements later.