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

kvrpc: deduplicate chained intercept by name #832

Merged
merged 12 commits into from
Jun 14, 2023

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Jun 9, 2023

deduplicate chained intercept by name so we can replace SetInterceptor with AddInterceptor

Signed-off-by: glorv <glorvs@163.com>
@glorv
Copy link
Contributor Author

glorv commented Jun 9, 2023

@disksing @nolouch PTAL

glorv added 7 commits June 9, 2023 15:04
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
@@ -903,6 +903,7 @@ func (s *KVSnapshot) SetRPCInterceptor(it interceptor.RPCInterceptor) {
}

// AddRPCInterceptor adds an interceptor, the order of addition is the order of execution.
// the chained interceptors will be dedupcated by its name.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need this?

Copy link
Contributor Author

@glorv glorv Jun 9, 2023

Choose a reason for hiding this comment

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

Currently, the same interceptor is set in different places in tidb, I'm not sure if they are set several times because the current behavior is replace. But I want to change the behavior to add

Signed-off-by: glorv <glorvs@163.com>
@@ -122,29 +153,49 @@ func NewRPCInterceptorChain() *RPCInterceptorChain {
// Link is used to link the next RPCInterceptor.
// Multiple interceptors will be executed in the order of link time.
Copy link
Member

Choose a reason for hiding this comment

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

add a comment about the behavior of linking duplicated interceptor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

rest LGTM

Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
@Connor1996
Copy link
Member

/merge

@Connor1996 Connor1996 merged commit 0b4b0ca into tikv:master Jun 14, 2023
@glorv glorv deleted the rpc-inteceptor branch June 14, 2023 03:26
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.

3 participants