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

Fix: error message using correct keepalive config value #7038

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

BatmanAoD
Copy link
Contributor

@BatmanAoD BatmanAoD commented Mar 14, 2024

kpTimeoutLeft is calculated from kp.Timeout, but the error message implies it's based on kp.Time.

RELEASE NOTES:

  • transport/server: use the proper timeout value when keepalive pings are not ack'd in time

`kpTimeoutLeft` is calculated from `kp.Timeout`, but the error message implies it's based on `kp.Time`.
Copy link

linux-foundation-easycla bot commented Mar 14, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: BatmanAoD / name: Kyle J Strand (5b3db47)

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Merging #7038 (5b3db47) into master (4f43d2e) will increase coverage by 0.04%.
Report is 19 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7038      +/-   ##
==========================================
+ Coverage   82.44%   82.48%   +0.04%     
==========================================
  Files         296      296              
  Lines       31475    31475              
==========================================
+ Hits        25948    25962      +14     
+ Misses       4471     4456      -15     
- Partials     1056     1057       +1     
Files Coverage Δ
internal/transport/http2_server.go 90.33% <100.00%> (-0.47%) ⬇️

... and 13 files with indirect coverage changes

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM, but you will need to sign off on the CLAs (by clicking the links from the comment from the bot). Thanks!

@BatmanAoD
Copy link
Contributor Author

@dfawley Talking to my company about it now; it will probably be a couple of weeks because this fix isn't actually particularly high-priority. (I should have just opened a ticket, since the fix is so trivial...)

@zasweq zasweq modified the milestones: 1.63 Release, 1.64 Release Mar 20, 2024
Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added stale and removed stale labels Mar 26, 2024
@BatmanAoD
Copy link
Contributor Author

I will be discussing the CLA with the appropriate person at my company on April 2nd.

Copy link

github-actions bot commented Apr 1, 2024

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added stale and removed stale labels Apr 1, 2024
@arvindbr8
Copy link
Member

@BatmanAoD -- Did your meeting happen? Hopefully it went well.

@BatmanAoD
Copy link
Contributor Author

@arvindbr8 It sounds like the CLA shouldn't be a problem for us; I've started the signature process.

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

Nice, now we can see more PR's from you @BatmanAoD :) . Thanks so much for the PR!

@arvindbr8 arvindbr8 merged commit c31cec3 into grpc:master Apr 3, 2024
14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants