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

Adds new NewClientWithAddressContextNoEnvTimeout func #437

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JoshVanL
Copy link
Contributor

As an app developer I do not want to have a default of 5 seconds timeout for connecting to daprd, and don't wish to use environment variables to configure this value. Instead, I would like to solely rely on the context I pass to the client New func.

Adds new NewClientWithAddressContextNoEnvTimeout func to create clientwhich ignores the DAPR_CLIENT_TIMEOUT_SECONDS env var.

which ignores the `DAPR_CLIENT_TIMEOUT_SECONDS` env var.

Signed-off-by: joshvanl <[email protected]>
@JoshVanL JoshVanL requested a review from a team as a code owner August 16, 2023 22:10
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4afb831) 70.00% compared to head (9e6b273) 70.04%.
Report is 22 commits behind head on main.

❗ Current head 9e6b273 differs from pull request most recent head 54abb81. Consider uploading reports for the commit 54abb81 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #437      +/-   ##
==========================================
+ Coverage   70.00%   70.04%   +0.04%     
==========================================
  Files          33       33              
  Lines        2667     2671       +4     
==========================================
+ Hits         1867     1871       +4     
  Misses        698      698              
  Partials      102      102              
Files Coverage Δ
client/client.go 70.73% <100.00%> (+0.98%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@artursouza artursouza left a comment

Choose a reason for hiding this comment

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

We have an approved proposal that let's users configure the timeout via env variable. The name has changed, to become a standard across SDKs. Can this env variable be kept and change the default to be "no timeout" instead of 5s?

@artursouza
Copy link
Member

Another options is to have the env var override the timeout for the context (if possible).

@JoshVanL
Copy link
Contributor Author

I'd be happy to have the default to be no timeout if we don't consider that a breaking change.

@artursouza
Copy link
Member

artursouza commented Aug 16, 2023 via email

@yaron2
Copy link
Member

yaron2 commented Aug 17, 2023

I have knowledge of Go-SDK users that rely on the default 5s timeout for various reasons, so I recommend to not change the default.

client/client.go Outdated Show resolved Hide resolved
@daixiang0
Copy link
Member

ping @JoshVanL

Signed-off-by: joshvanl <[email protected]>
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.

4 participants