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.

Sign up for GitHub

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

Jump to bottom

transport: server keepalives should be sent every [Time] period #3172

Merged
merged 2 commits into from Nov 19, 2019
Merged

transport: server keepalives should be sent every [Time] period #3172

merged 2 commits into from Nov 19, 2019

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Nov 12, 2019

This PR contains the server side changes corresponding to the client
side changes made in #3102.

Apart from the fix for the issue mentioned in
#2638, this PR also makes some
minor code cleanup and fixes the channelz test for keepalives count.

@easwars
server: Keepalive pings should be sent every [Time] period
35cdb1b 
This PR contains the server side changes corresponding to the client
side changes made in grpc#3102.

Apart from the fix for the issue mentioned in
grpc#2638, this PR also makes some
minor code cleanup and fixes the channelz test for keepalives count.
@easwars easwars requested a review from dfawley November 12, 2019 03:13
@easwars easwars added this to the 1.26 Release milestone Nov 12, 2019
dfawley
dfawley reviewed Nov 18, 2019
View reviewed 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.

Changes look good. Can you run a QPS style benchmark (many concurrent, small RPCs) for this and report the numbers? Thanks!

internal/transport/http2_server.go Outdated
// acked).
sleepDuration := minTime(t.kp.Time, kpTimeoutLeft)
kpTimeoutLeft -= sleepDuration
prevNano = lastRead
Copy link
Member

Choose a reason for hiding this comment

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

I this case I think we know prevNano == lastRead already (otherwise the first if above would have skipped this), so this is unnecessary. Or lastRead is going backwards which is 😮 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Benchmark config::

Mode: Unary
BenchTime: 1m
MaxConcurrentCalls: 100
ReqSize: 1B
RespSize: 1B

Benchmark results::

           Title       Before        After Percentage
        TotalOps      2240700      2237780    -0.13%
         SendOps            0            0      NaN%
         RecvOps            0            0      NaN%
        Bytes/op      8491.83      8492.49     0.01%
       Allocs/op       161.22       161.28     0.00%
         ReqT/op    298760.00    298370.67    -0.13%
        RespT/op    298760.00    298370.67    -0.13%
        50th-Lat   2.601787ms   2.619027ms     0.66%
        90th-Lat   3.766543ms   3.750905ms    -0.42%
        99th-Lat   5.282168ms   5.185314ms    -1.83%
         Avg-Lat   2.676142ms   2.679648ms     0.13%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you say lastRead is going backwards?
Looks like I added this assignment in the client side PR because we wanted to avoid a crazy race. But thinking about it now, it looks like we don't need to make the assignment. I'm removing it here and in the client side.

Copy link
Member

Choose a reason for hiding this comment

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

Why do you say lastRead is going backwards?

I didn't say it is. I said the only way we can reach this point is if lastRead is either unchanged or going backwards. I assumed the latter wasn't possible, so lastRead == prevNano and there's no need for this assignment.

@dfawley dfawley assigned easwars and unassigned dfawley Nov 19, 2019
@easwars easwars assigned dfawley and unassigned easwars Nov 19, 2019
@easwars easwars merged commit d720ab3 into grpc:master Nov 19, 2019
@easwars easwars deleted the keepalive_server branch November 19, 2019 22:44
RealBar pushed a commit to RealBar/grpc-go that referenced this pull request Nov 21, 2019
@easwars @RealBar
server: Keepalive pings should be sent every [Time] period (grpc#3172)
1fbd10b 
This PR contains the server side changes corresponding to the client
side changes made in grpc#3102.

Apart from the fix for the issue mentioned in
grpc#2638, this PR also makes some
minor code cleanup and fixes the channelz test for keepalives count.
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
@easwars easwars changed the title server: Keepalive pings should be sent every [Time] period transport: server keepalives should be sent every [Time] period Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

Assignees

@dfawley dfawley

Labels
Type: Bug
Projects
None yet
Milestone
1.26 Release
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@easwars @dfawley