-
Notifications
You must be signed in to change notification settings - Fork 791
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
[CLI] fix a few things in domain migration command #5374
Conversation
@@ -139,6 +135,31 @@ func (b *clientFactory) ServerAdminClient(c *cli.Context) admin.Client { | |||
return admin.NewThriftClient(serverAdmin.New(clientConfig)) | |||
} | |||
|
|||
// ServerFrontendClientForMigration builds a frontend client (based on server side thrift interface) | |||
func (b *clientFactory) ServerFrontendClientForMigration(c *cli.Context) frontend.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.
the code is exactly the same as ServerFrontendClient
, should dispatcher
be replaced with something else?
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.
good catch. fixed
tools/cli/factory.go
Outdated
// ServerAdminClientForMigration builds an admin client (based on server side thrift interface) | ||
func (b *clientFactory) ServerAdminClientForMigration(c *cli.Context) admin.Client { | ||
b.ensureDispatcher(c) | ||
clientConfig := b.dispatcher.ClientConfig(cadenceFrontendService) |
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.
same as above
} | ||
|
||
// DEPRECATED don't use, only reserved for backward compatibility purposes | ||
// NewClientFactory creates a new ClientFactory | ||
func NewClientFactory() ClientFactory { |
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.
The method was marked as deprecated, is there a way to not use it?
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 was deprecated in the previous CLI pr but then I realize it should not be deprecated.
b.dispatcher = b.newClientDispatcher(c, c.GlobalString(FlagAddress)) | ||
} | ||
|
||
func (b *clientFactory) ensureDispatcherForMigration(c *cli.Context) { |
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.
this method is not used anywhere
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.
used now
@@ -768,8 +768,8 @@ func newIndefiniteContext(c *cli.Context) (context.Context, context.CancelFunc) | |||
} | |||
|
|||
func newTimedContext(c *cli.Context, timeout time.Duration) (context.Context, context.CancelFunc) { | |||
if c.GlobalIsSet(FlagContextTimeout) { |
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 this operation is not thread safe, I changed it
What changed?
Why?
add two methods in ClientFactory for migration
This is needed to be able to use environment flags for internal usage
How did you test it?
Potential risks
No Risk
Release notes
Documentation Changes