-
Notifications
You must be signed in to change notification settings - Fork 220
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
Deprecate NewClient, add Dial and NewLazyClient and CheckHealth #795
Conversation
NewClient
, add Dial
and NewLazyClient
and CheckHealth
NewClient
, add Dial
and NewLazyClient
and CheckHealth
// We intentionally don't "ensureIntialized" here because there is no direct | ||
// error return path. Rather we let GetWorkflowHistory do 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.
I'd avoid words "we" and "I" in comments. Something more "dry" is better here:
// We intentionally don't "ensureIntialized" here because there is no direct | |
// error return path. Rather we let GetWorkflowHistory do it. | |
// "ensureIntialized" is not called here intentionally because there is no direct | |
// error return path. GetWorkflowHistory will do it. | |
// to the server eagerly and will return an error if the server is not | ||
// available. | ||
func Dial(options Options) (Client, error) { | ||
return internal.DialClient(options) |
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.
return internal.DialClient(options) | |
return internal.Dial(options) |
?
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.
Just Dial
in a client
package makes sense, but just Dial
in an internal package where you could be dialing other things does not. worker.New
-> internal.NewWorker
, client.Options
-> internal.ClientOptions
, etc, etc.
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 okay with it because the functionality is there but I had different expectations.
I had understood that all SDKs would move to become lazy by default with an option to be eager and thus I expected Dial to be lazy and NewClient to remain eager (to be kind to existing callers who might expect certain behavior). Not sure that NewLazyClient pulls its own weight in terms of utility versus adding to the API surface area.
// Dial creates an instance of a workflow client. This will attempt to connect | ||
// to the server eagerly and will return an error if the server is not | ||
// available. | ||
func Dial(options Options) (Client, 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.
Curous if you considered taking this opportunity to move to function-style optional args?
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.
For most of the Go SDK, the struct options is good enough to still give us future proofing. There's little benefit of a func Dial(options ...DialOption) (Client, error)
over struct-based if you 1) have no other params, 2) don't need to preserve backwards compatibility, and 3) can reasonably copy the struct.
I could be leaving off an important benefit, so if there is a good enough argument I'd definitely reconsider.
What was changed
Mostly what's at #793 w/ slight changes.
client.Dial
which eagerly connects likeNewClient
doesclient.NewLazyClient
which doesn't connect until you make a callNewClient
client.Client.CheckHealth
callWhy?
People want lazy-connecting clients but we still need capabilities checks. People also wanted explicit health checking.
Checklist