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

TCG-2460 - Automatically refresh frequently used keys before they expire #24

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

guywithnose
Copy link
Contributor

@guywithnose guywithnose commented Mar 21, 2020

What does this PR do?

TCG-2460

Checklist

  • [ ] Pull request contains a clear definition of changes
  • [ ] Tests (either unit, integration, or acceptance) written and passing
  • [ ] Relevant documentation produced and/or updated

@guywithnose guywithnose requested a review from a team as a code owner March 21, 2020 22:04
src/Predis.php Show resolved Hide resolved
src/Predis.php Outdated Show resolved Hide resolved
src/Predis.php Outdated Show resolved Hide resolved
@chadicus
Copy link
Contributor

In my opinion the functionality you are adding should not be in the Predis class, but instead be part of a custom ClientInterface instance.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f7a7a21 on guywithnose:master into 0e8a29e on traderinteractive:master.

@coveralls
Copy link

coveralls commented Mar 23, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 3114e9b on guywithnose:master into 0e8a29e on traderinteractive:master.

@guywithnose guywithnose changed the title TCG-2460 - Prevent cache stampeding TCG-2460 - Automatically refresh frequently used keys before they expire Apr 1, 2020
@guywithnose
Copy link
Contributor Author

I pulled out the locking part, because as @chadicus pointed out that may work better at a different abstraction level. The part that measures runtime still needs to be here, so I left that.

@guywithnose
Copy link
Contributor Author

@chadicus Here is where I moved the locking part: https://github.com/traderinteractive/lib-predis-anti-stampede-php/pull/1

Copy link
Contributor

@chrisryan chrisryan left a comment

Choose a reason for hiding this comment

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

This looks good to me. I have two thoughts/considerations depending on how we want to approach backward compatibility.

  1. Should the default refreshPercent be 0 so the behavior does not change unexpectedly in cases where the refresh is not desirable for some reason?
  2. There could be an argument that these changes should be in a wrapper or extended class which would avoid the first issue.

I think the first issue would be easy enough but understand it would require us to do more work than just upgrade the library. I am open to other thoughts/opinions.

@guywithnose
Copy link
Contributor Author

I set refreshPercent to default to 0. That should make it fully BC

@guywithnose guywithnose merged commit c018263 into traderinteractive:master Apr 27, 2020
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.

4 participants