-
Notifications
You must be signed in to change notification settings - Fork 246
feat: support connection lifetime for single client #727
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
f18e613
feat: add connection lifetime option to single client
terut f950c1e
Remove mutex and timer flag for connection lifetime timer
terut 390e19b
Retry wire accquition when failed to stop connection lifetime timer
terut 88c8d7e
Add timer test to pipe
terut 14349d4
Add test for reseting timer and stopping timer when using pool
terut e91d316
Remove p.StopTimer() from p.Close()
terut 9fba892
Forced to retry when errConnExpired
terut c0c3657
Remove hasConnLftm and check resps[0] to retry for multi cmds
terut 31de820
Merge branch 'main' into feat/conn-lifetime
terut be0ab03
Recover connection lifetime error in the middle of calls
terut 7c6be89
Fix the handling of connection lifetime error of DoMultiCache
terut 462c44e
perf: apply fieldaligments
rueian File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that
errConnExpiredhappens in the middle ofDoMulti? I am not sure, but If it is possible then we should not retry preceding requests that don't receive the error.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think it's unlikely. Surely all responses have same error when changing
p.state.I will change that like the following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed c0c3657
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, could you leave a comment in the code to explain why it won't happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I was checking the behavior of connection lifetime on concurrent process and then I found the error
read tcp [::1]:35190->[::1]:6379: use of closed network connectionthroughsingleClient.DoMulti. I think probably the error occurred because ofpipe.Close(), but the investigation isn't going well. Could you advice me for that? @rueianThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rueian Thanks! As you said, it looks like that
_backgroundReadreturns that error.I will change that lines to return
errConnExpiredwhen expired and thenerrConnExpiredhappens in the middle ofDoMulti, so we may should check the error of all response.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @terut, any update?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for late. I had no time to spare... I think I can work on this problem from this week. Anyway, I will merge any updates of connection pools. @rueian
I have a feeling that probably this function is only for read replica, right? It seems like write cmds don't work on this approach. Sometime incremented value is 10001, 10002 and so on when loop is 10000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries.
What do you mean by this? I think this can be a general feature for those who want a limited lifetime on each connection for any reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I just counted up for 10000 times using connection lifetime option, the value of keys is over 10000. But my implementation is not correct for now at the point of view of the error handling of
_backgroundRead, I will try to count up again after implementing correctly.