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:registry ping timeout #808

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Conversation

likhita-8091
Copy link
Contributor

I see that registry ping does not have timeout handling. The makeFetcher function clearly has a ctx timeout handling. Why is it not set during ping? I currently set the easiest 3 second timeout.

@google-cla
Copy link

google-cla bot commented Nov 4, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@likhita-8091
Copy link
Contributor Author

感谢您的请求。看来这可能是您对Google开源项目的第一个贡献(如果没有,请在下面寻求帮助)。在我们查看您的拉取请求之前,您需要签署贡献者许可协议(CLA)。

📝 请访问https://cla.developers.google.com/进行签名。

签署(或解决任何问题)后,请在此处回复@googlebot I signed it!,我们将对其进行验证。

如果您已经签署了CLA,该怎么办

个人签名人
公司签名人

ℹ️ Google员工:请点击此处获取更多信息

@googlebot I signed it!

@codecov-io
Copy link

Codecov Report

Merging #808 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #808   +/-   ##
=======================================
  Coverage   74.91%   74.91%           
=======================================
  Files         103      103           
  Lines        4309     4309           
=======================================
  Hits         3228     3228           
  Misses        605      605           
  Partials      476      476           
Impacted Files Coverage Δ
pkg/v1/remote/transport/ping.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb6ca2e...06f7ac8. Read the comment docs.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

I am okay with this being hardcoded to 2s for now, but that might be too low for people on really unreliable/slow internet. If we see that happening, I may make this configurable or just increase the timeout.

@jonjohnsonjr jonjohnsonjr merged commit ca90ba6 into google:master Nov 10, 2020
@jonjohnsonjr
Copy link
Collaborator

Thanks!

@likhita-8091 likhita-8091 deleted the ping_timeout branch November 11, 2020 02:00
@likhita-8091
Copy link
Contributor Author

Of course, 2s is indeed a bit short, because I just changed it briefly and temporarily. I hope you can change the specific configuration.

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.

None yet

3 participants