Closed ariasmn closed 3 years ago
Thank you for this PR! I'll have a look and test this PR on this weekend. 👍
Hi, thanks for the reply and don't worry, hope everything is ok!
I get what you are saying, but I don't know the correct way of implementing it. I guess that, since only Storage has access to the adapter, something like this should be done (?):
public function clearSuccessCount(string $service): void
{
if ($this->adapter instanceof Redis) {
$this->adapter->removeExpiredElements($this->successKey($service));
}
}
Because we have to control whether or not the adapter has the removeExpiredElements method before calling it, obviously.
Is there a cleaner/better way to do so? I don't know, maybe adding a new method that works similarly to the supportCountStrategy method, and maybe allowing to set if to false
, just in case anyone wants to preserve the expired elements? :thinking:
Waiting for your response. Once I get it back, I will try to modify the PR ASAP to satisfy the requirements.
Thank you and stay safe!
@ariasmn I'm sorry I didn't explain it enough. 🙇
My idea is that removing the expired values (zRemRangeByScore) in Redis::increment()
instead of Redis::load()
.
It is similar to the implementation you have shown here, it runs zRemRangeByScore
, not expire
.
Thank you for your sincere response. Please let me know if you have any questions. ✨
@ackintosh Thank you, everything is clear now!
I have updated the PR to meet the requeriments you pointed (truth be told, is way cleaner now).
If anything should be done before merging the PR, please hit me up and I will try my best to do it as fast as I can.
https://github.com/ackintosh/ganesha/pull/74/checks?check_run_id=2545653270
1) Ackintosh\Ganesha\StorageTest::getLastFailureTimeWithSlidingTimeWindow Error: Call to a member function timeWindow() on null
Oh, the CI failure is due to lack of adapter setup in unit tests. 👀
Setting up Redis adapter with calling setContext()
like here should fix the CI failure.
+ $redis = new Redis(($r));
+ $context = /* *** */
+ $redis->setContext($context);
+ $storage = new Storage($redis, new Ganesha\Storage\StorageKeys(), null);
- $storage = new Storage(new Redis(($r)), new Ganesha\Storage\StorageKeys(), null);
@ackintosh Woops, my bad.
Tests are fixed now and they passed locally. If anything is wrong, please just tell me!
Thank you! I'm checking the adapter manually using the example scripts. Please give me some time. 🙏
Hmm... I've found out that when a remote service can not respond any request, success count is not cleared (due to Redis::increment()
is not called with success key).
So we need to clear expired values also in Redis::load()
in addition to Redis::increment()
.
Sorry for my mistake. 😓
@ackintosh No problem. Also my bad, didn't even realize that while testing the new implementation :sweat:
Now the expired values also get cleared in Redis::load()
. Also added its tests.
Tell me if anything else should be done!
v1.2.3, contains this fix has been released. 🎉
@ariasmn Thank you again for your great work! ✨
@ariasmn Btw, may I add your company/project as a user in the README? If it's okay with you, please let me know. 😃
Implemented only for the Redis adapter. Others adapter's methods are empty.
The method gets called everytime
Ganesha::isAvailable()
, which was the wanted behaviour, but the cleanup wasn't being executed correctly.. When called, the method remove those elements in the success keys which were registered before the time set in thetimeWindow()
(so, keys created before now - timeWindow get removed).This change, as far as I can tell (correct me if I'm wrong), doesn't break the behaviour, neither the implementations with adapters other than the Redis one.