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

Add frontend DC redirection functionality and policy #1409

Merged
merged 8 commits into from
Jan 26, 2019

Conversation

wxing1292
Copy link
Contributor

No description provided.

@wxing1292 wxing1292 requested a review from samarabbas January 23, 2019 23:22
@wxing1292 wxing1292 changed the title Add Frontend DC redirection policy Add Frontend DC redirection functionality and policy Jan 23, 2019
@wxing1292 wxing1292 changed the title Add Frontend DC redirection functionality and policy Add frontend DC redirection functionality and policy Jan 23, 2019
@wxing1292 wxing1292 force-pushed the client-redirection-policy branch from 37e3831 to 140c70c Compare January 24, 2019 21:54
@wxing1292 wxing1292 force-pushed the client-redirection-policy branch from 140c70c to 62337c9 Compare January 24, 2019 22:00
@@ -45,6 +46,7 @@ type (
GetHistoryClient() history.Client
GetMatchingClient() matching.Client
GetFrontendClient() frontend.Client
GetPublicClient() public.Client
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

wh.domainCache,
wh.dcRedirectionPolicy,
)
dcRediectionHandle := NewDCRedirectionHandler(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo 'dcRediectionHandle'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

dcRediectionHandle := NewDCRedirectionHandler(
currentClusteName, dcRedirectionPolicy, wh.Service, wh,
)
wh.Service.GetDispatcher().Register(workflowserviceserver.New(dcRediectionHandle))
Copy link
Contributor

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.

Copy link
Contributor Author

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
}

Copy link
Contributor

@samarabbas samarabbas left a 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.

@wxing1292 wxing1292 merged commit dc2d9bf into master Jan 26, 2019
@wxing1292 wxing1292 deleted the client-redirection-policy branch February 8, 2019 18:24
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.

2 participants