-
Notifications
You must be signed in to change notification settings - Fork 814
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
Add frontend DC redirection functionality and policy #1409
Conversation
37e3831
to
140c70c
Compare
140c70c
to
62337c9
Compare
@@ -45,6 +46,7 @@ type ( | |||
GetHistoryClient() history.Client | |||
GetMatchingClient() matching.Client | |||
GetFrontendClient() frontend.Client | |||
GetPublicClient() public.Client |
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.
FrontendClient and PublicClient are same thing. Why do we need separate definitions?
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.
because each time the idl on the frontend is updated, developer will have to update the public client first, then get it approved, then update the public sever repo pointing to public client, even for developing features.
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.
tried to use client IDL (revert back to prev impl: frontend depends on public client idl)
this will break the redirection handler: although the definition contents are exactly the same, the package name are different. this leads to only 2 solution: either change the idl definition dependency of frontend handler, which will break a lot of code, or use the solution in this PR
created a issue #1414, so in the future, client idl and service idl will be in the same repo
service/frontend/workflowHandler.go
Outdated
wh.domainCache, | ||
wh.dcRedirectionPolicy, | ||
) | ||
dcRediectionHandle := NewDCRedirectionHandler( |
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.
nit: typo 'dcRediectionHandle'
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.
fixed
service/frontend/workflowHandler.go
Outdated
dcRediectionHandle := NewDCRedirectionHandler( | ||
currentClusteName, dcRedirectionPolicy, wh.Service, wh, | ||
) | ||
wh.Service.GetDispatcher().Register(workflowserviceserver.New(dcRediectionHandle)) |
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'm a little worried that we always register the redirect handler. I think RedirectHandler needs to wrap WorkflowHandler and only registered when we need forwarding.
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.
take a look at the noop redirection policy, which is only 6 lines of code (see below).
if we trust that the redirection handler works, we should also trust the code below.
// GetTargetDatacenterByName get target cluster name by domain Name
func (policy *NoopRedirectionPolicy) GetTargetDatacenterByName(domainName string) (string, error) {
return policy.currentClusteName, nil
}
// GetTargetDatacenterByID get target cluster name by domain ID
func (policy *NoopRedirectionPolicy) GetTargetDatacenterByID(domainID string) (string, error) {
return policy.currentClusteName, nil
}
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.
looks good. You can land this after addressing my 2 comments.
No description provided.