grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

Published 20 hours ago • verified publisher icon grpc

test/keepalive: Improve TestKeepaliveClientClosesWithActiveStreams

Open tobotg opened this issue 2 years ago • 9 comments

Setup a ping/noping switchable server to use in KeepaliveClientClosesWithActiveStreams test

Fixes #6099

RELEASE NOTES: N/A

tobotg avatar May 15 '23 18:05 tobotg

@arvindbr8 Hello, I'm unsure about what caused Testing / vet-proto (pull_request) above to fail and how to fix it for this PR. Is it something you could help me with? Thanks in advance.

tobotg avatar May 15 '23 18:05 tobotg

Hello, I'm unsure about what caused Testing / vet-proto (pull_request) above to fail

This one is not actionable in your PR. And I believe extras should be fixed if you fetch master@HEAD and rebase+force-push (or merge into?) your branch.

dfawley avatar May 15 '23 23:05 dfawley

Thanks @dfawley It helped. However, are Test/DialWaitsForServerSettingsAndFails and Test/FullHandshake known flaky? They failed in the last run (one in Testing / tests (tests, 1.20) (pull_request) and the other in Testing / tests (tests, 1.20, arm64) (pull_request) ), but they are not related to the fix I pushed. Any thoughts?

tobotg avatar May 16 '23 11:05 tobotg

Hey @tobotg Test/DialWaitsForServerSettingsAndFails and Test/FullHandshake are not known flakes. I've triggered a re-run. lets see how that goes.

arvindbr8 avatar May 17 '23 17:05 arvindbr8

Hey @arvindbr8 all the tests passed now, so it's. btw, thanks for the review. I'll look into it and get back here.

tobotg avatar May 17 '23 19:05 tobotg

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[bot] avatar May 23 '23 20:05 github-actions[bot]

I've been very busy lately, hopefully I'll be able to get back to this during the weekend.

tobotg avatar May 23 '23 20:05 tobotg

Hey @arvindbr8 , I gave it another try. Please have a look when you get a chance. Thanks

tobotg avatar May 30 '23 20:05 tobotg

@arvindbr8 is out of town for a couple weeks; will resume when he returns.

dfawley avatar May 31 '23 20:05 dfawley

Hey @tobotg, thanks for the revision. I will take a look at it shortly.

arvindbr8 avatar Jun 15 '23 15:06 arvindbr8

thanks @tobotg, Changes looks better now. A few minor comments and nits. Please take a look and let us know.

arvindbr8 avatar Jun 20 '23 16:06 arvindbr8

Thanks @arvindbr8 for the review. I had a look and addressed the changes you requested. Please let me know how it looks now.

tobotg avatar Jun 20 '23 23:06 tobotg

@tobotg, Please let us know when you want us to take another look at this.

arvindbr8 avatar Jun 27 '23 22:06 arvindbr8

@arvindbr8 Thanks for the review (and I added your comment). Please take another look when you get a chance.

tobotg avatar Jun 28 '23 06:06 tobotg

@arvindbr8 All passed now. @dfawley

tobotg avatar Jun 30 '23 00:06 tobotg

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[bot] avatar Jul 30 '23 16:07 github-actions[bot]

@dfawley hopefully I'll be able to get back to this during the weekend and address your comments. Thanks

tobotg avatar Aug 02 '23 13:08 tobotg

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[bot] avatar Aug 08 '23 16:08 github-actions[bot]

Back to it. I'll submit a new commit this week. Thanks

tobotg avatar Aug 15 '23 13:08 tobotg

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[bot] avatar Aug 21 '23 16:08 github-actions[bot]

I made changes, and I'll submit later after tests. Thanks

tobotg avatar Aug 28 '23 08:08 tobotg

@dfawley Hello! I just submitted a new commit now, I hope it captured the feedback you shared recently. However, I'm unsure what the vet issue is here, any clue will be appreciated.Thanks

tobotg avatar Aug 29 '23 13:08 tobotg

If you rebase your branch to master, the vet issue should get fixed. Either way, it does not affect the ability to merge the changes once approved.

easwars avatar Aug 29 '23 16:08 easwars

If you rebase your branch to master

In general, it's best to not rebase a PR from master until the reviews are done, as it messes up github diffs and review comment threads.

In this particular case, it's not necessary at all, because the vet-proto checker is optional and the failure is because external/unrelated things are out of sync that we will fix separately.

dfawley avatar Aug 29 '23 20:08 dfawley

Hello @dfawley and @arvindbr8 Just in case I missed it, I wanted to check in if there is any clarification or task needed from me on this PR before review. Thanks

tobotg avatar Sep 24 '23 15:09 tobotg

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[bot] avatar Oct 03 '23 02:10 github-actions[bot]

Thanks @arvindbr8 for the review. I'll look into it during this weekend and get back to you if more elaboration needed.

tobotg avatar Oct 05 '23 09:10 tobotg

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[bot] avatar Oct 11 '23 22:10 github-actions[bot]