Skip to content
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

txnkv: add callback for setting ResourceGroupTag #368

Merged
merged 11 commits into from
Nov 15, 2021

Conversation

mornyx
Copy link
Contributor

@mornyx mornyx commented Nov 3, 2021

What problem does this PR solve?

Use callback function to generate ResourceGroupTag instead of passing it directly. In this way, we can expand ResourceGroupTag more flexibly on demand in TiDB, and be transparent to client-go.

What is changed and how it works?

  • Added ResourceGroupTagger, which is called to generate tag when ResourceGroupTag is nil.

Signed-off-by: mornyx <mornyx.z@gmail.com>
Signed-off-by: mornyx <mornyx.z@gmail.com>
@mornyx
Copy link
Contributor Author

mornyx commented Nov 3, 2021

PTAL, Thanks~
/cc @breeswish @zhongzc @crazycs520

Upstream PR: pingcap/tidb#29044.

@mornyx mornyx marked this pull request as ready for review November 3, 2021 13:38
@mornyx mornyx marked this pull request as draft November 3, 2021 13:59
Signed-off-by: mornyx <mornyx.z@gmail.com>
@mornyx mornyx marked this pull request as ready for review November 3, 2021 16:29
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@disksing disksing enabled auto-merge (squash) November 4, 2021 03:37
@disksing disksing disabled auto-merge November 4, 2021 03:41
Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like this (feel free to discuss):

type ResourceGroupTagger interface {
    tagCleanup(c *twoPhaseCommitter, bo *retry.Backoffer, batch batchMutations) []byte
    tagCommit(...)
    tagPrewrite(...)
    tagScan(...)
    tagGet(...)
    ....
}

or this:

type ResourceGroupTagger func(r *Request)

or even this:

type RequestPostBuildFn func(r *Request)

or:

type RequestPreSendFn func(r *Request)

@disksing What do you think?

util/resource_tag.go Outdated Show resolved Hide resolved
@zhongzc
Copy link

zhongzc commented Nov 8, 2021

How about something like this (feel free to discuss):

type ResourceGroupTagger interface {
    tagCleanup(c *twoPhaseCommitter, bo *retry.Backoffer, batch batchMutations) []byte
    tagCommit(...)
    tagPrewrite(...)
    tagScan(...)
    tagGet(...)
    ....
}

or this:

type ResourceGroupTagger func(r *Request)

or even this:

type RequestPostBuildFn func(r *Request)

or:

type RequestPreSendFn func(r *Request)

@disksing What do you think?

ResourceGroupTagger voted.

Signed-off-by: mornyx <mornyx.z@gmail.com>
@breezewish
Copy link
Member

Generally LGTM. However I believe there should be at least some unit tests to ensure that:

  1. When ResourceGroup is set, it appears in the final request.
  2. When ResourceGroupTagger is set, its modification take effects in the final request.
  3. When ResourceGroup and ResourceGroupTagger are both set, ResourceGroupTagger does not take effect (according to your logic).

@mornyx mornyx changed the title txnkv: replace ResourceGroupTag with factory function txnkv: add callback for setting ResourceGroupTag Nov 9, 2021
Signed-off-by: mornyx <mornyx.z@gmail.com>
@breezewish
Copy link
Member

@mornyx I opened an empty PR and seems that integration tests work fine #372

@mornyx
Copy link
Contributor Author

mornyx commented Nov 11, 2021

@mornyx I opened an empty PR and seems that integration tests work fine #372

Got, seems that problem occurred in the use of mock store in the new integration tests, I will fix it.

@mornyx
Copy link
Contributor Author

mornyx commented Nov 11, 2021

@mornyx I opened an empty PR and seems that integration tests work fine #372

The problem did appear in my own integration test. Because of the incorrect use of other component, the error message did not occur in my code. Now I have fixed it (at least it works on my local machine, Github actions need to wait too long...), and next I need to find a way to fix Compatibility Test.

@breezewish
Copy link
Member

breezewish commented Nov 12, 2021

I've checked with a last merged PR 8a4ce38 the compatibility test also failed and it is merged anyway. I think we can ignore it and focus on other tests.

@breezewish
Copy link
Member

breezewish commented Nov 12, 2021

@disksing Please take a look~ Thanks! All tests (except for the compatibility test) are now passing.

@disksing disksing merged commit 3b9f591 into tikv:master Nov 15, 2021
@mornyx mornyx deleted the resource-metering branch November 18, 2021 09:25
MyonKeminta pushed a commit to MyonKeminta/client-go that referenced this pull request Dec 13, 2021
* Replace resourceGroupTag with resourceGroupTagFactory(firstKey)

Signed-off-by: mornyx <mornyx.z@gmail.com>

* Fix npe

Signed-off-by: mornyx <mornyx.z@gmail.com>

* Abstract ResourceGroupFactory

Signed-off-by: mornyx <mornyx.z@gmail.com>

* Optimized as ResourceGroupTagger

Signed-off-by: mornyx <mornyx.z@gmail.com>

* Add test for resource group tagger

Signed-off-by: mornyx <mornyx.z@gmail.com>

* Fix unexpected request failed in mock store

Signed-off-by: mornyx <mornyx.z@gmail.com>

* Fix integration tests

Signed-off-by: mornyx <mornyx.z@gmail.com>

Co-authored-by: Wenxuan <breezewish@pingcap.com>
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants