Call RedirectionInterceptor only for WorkflowHandler #3924
Conversation
| if _, isWorkflowHandler := info.Server.(*WorkflowHandler); !isWorkflowHandler { | ||
| return handler(ctx, req) | ||
| } |
There was a problem hiding this comment.
Is this necessary? All requests will have this extra check.
There was a problem hiding this comment.
I believe this is very cheap check. The other option is to compare strings: first return value from SplitMethodName with hardcoded string temporal.api.workflowservice.v1.WorkflowService but I believe checking type is better and faster.
There was a problem hiding this comment.
no strong opinion. however, admin and operator are mostly manual trigger requests. most of the requests are workflow service requests.
There was a problem hiding this comment.
Don't forget health check. I see tons of unknown API: Check warning logs.
There was a problem hiding this comment.
I don't have a strong opinion between this type check and string check either. but I agree we should do one since the redirection logic only applies to workflowservice
| info *grpc.UnaryServerInfo, | ||
| handler grpc.UnaryHandler, | ||
| methodName string, | ||
| respAllocFn func() interface{}, | ||
| respCtorFn responseConstructorFn, |
There was a problem hiding this comment.
This is reflection of my nostalgic memories from the past years: https://stackoverflow.com/questions/4614099/what-is-the-meaning-of-ctor.
| @@ -45,75 +45,75 @@ import ( | |||
| var ( | |||
| dcRedirectionMetricsPrefix = "DCRedirection" | |||
|
|
|||
| localAPIResults = map[string]respAllocFn{ | |||
| localAPIResponses = map[string]responseConstructorFn{ | |||
There was a problem hiding this comment.
now that I'm looking at this, have we benchmarked this against the equivalent reflection-based code? I see we need the maps anyway but they could just be sets and use reflection to construct the response. I'm curious about code bloat effects
There was a problem hiding this comment.
I also thought about it. I even have a func for it:
func newOfSameType(val reflect.Value) reflect.Value {
valType := val.Type().Elem()
newValue := reflect.New(valType)
val.Set(newValue)
return newValue
}but as soon as map is needed anyway, I am in doubt. Best thing, I guess, is code-gen the map.
There was a problem hiding this comment.
not quite that easy, you have to go from grpc method name to first return value of that method on WorkflowService. but only like 3-4 more lines.
we can't just code-gen, the maps hold the semantics about which calls need to be forwarded. but they could be map[string]struct{}, and then use reflection to make the response values
| if _, isWorkflowHandler := info.Server.(*WorkflowHandler); !isWorkflowHandler { | ||
| return handler(ctx, req) | ||
| } |
There was a problem hiding this comment.
I don't have a strong opinion between this type check and string check either. but I agree we should do one since the redirection logic only applies to workflowservice
What changed?
Call
RedirectionInterceptoronly forWorkflowHandler.Why?
RedirectionInterceptoris not intended to be used by other thanWorkflowHandlerhandler.How did you test it?
Existing tests.
Potential risks
No risks.
Is hotfix candidate?
No.