-
Notifications
You must be signed in to change notification settings - Fork 917
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
Reimplement DC redirection handler #3887
Conversation
* Implement generic DC redirection interceptor & UT
@@ -26,9 +26,9 @@ package interceptor | |||
|
|||
import "strings" | |||
|
|||
func splitMethodName( | |||
func SplitMethodName( |
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.
Seem like this is just AbstractMethodName? Only the second result is used?
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.
we only use the second half for now
i.logger.Warn(fmt.Sprintf("RedirectionInterceptor encountered unknown API: %v", methodName)) | ||
return handler(ctx, req) |
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.
could it assume there are only two sets of APIs? can it do return i.handleLocalAPIInvocation(ctx, req, handler, methodName)
?
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 can but will looks weird
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.
handler grpc.UnaryHandler, | ||
methodName string, | ||
) (_ interface{}, retError error) { | ||
scope, startTime := i.beforeCall(dcRedirectionMetricsPrefix + methodName) |
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.
only method name if it is local invocation?
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.
what do you mean?
@@ -81,7 +84,7 @@ func NewClientBean(factory Factory, clusterMetadata cluster.Metadata) (Bean, err | |||
} | |||
|
|||
adminClients := map[string]adminservice.AdminServiceClient{} |
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.
Do we never need to do redirection for admin service calls?
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 do not believe there is such need.
methodName string, | ||
respAllocFn func() interface{}, | ||
namespaceName namespace.Name, | ||
) (_ interface{}, retError error) { |
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.
Do we need/want to avoid multiple redirects? e.g. to prevent a request from bouncing between cells.
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.
request will only bounce between clusters if namespace flip flop
What changed?
Why?
Reduce repetitive logic
How did you test it?
UT
Potential risks
N/A
Is hotfix candidate?
N/A