Success count now gets cleared when using Redis

[ariasmn]

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 the timeWindow() (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.

[coveralls]

Coverage decreased (-0.3%) to 91.447% when pulling bbef016 on ariasmn:redis-success-cleanup into aa4fa2b on ackintosh:master.

[ackintosh]

Thank you for this PR! I'll have a look and test this PR on this weekend. 👍

[ackintosh]

Sorry for replying so late. 🙇

I think it is better to make changes only in the Redis Adapter, not adding abstract method to AdapterInterface. 💡

---

As we discussed here, zAdd does not allow to set an expire value, then we should remove expired elements explicitly.

However, there are cases in which the remove is unnecessary: using storages which allows expire value (e.g. memcached), or using Count strategy.

I think it is better to make changes only in the Redis Adapter. By doing so we can avoid adding methods to Adapters that we do not need to implement.

[ariasmn]

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? 🤔

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!

[ackintosh]

@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. ✨

[ariasmn]

@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.

[ackintosh]

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. 👀

---

ganesha/tests/Ackintosh/Ganesha/StorageTest.php

Line 47 in 1d568b5

$storage = new Storage(new Redis(($r)), new Ganesha\Storage\StorageKeys(), null);

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); 

[ariasmn]

@ackintosh Woops, my bad.

Tests are fixed now and they passed locally. If anything is wrong, please just tell me!

[ackintosh]

Thank you! I'm checking the adapter manually using the example scripts. Please give me some time. 🙏

[ackintosh]

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. 😓

[ariasmn]

@ackintosh No problem. Also my bad, didn't even realize that while testing the new implementation 😓
Now the expired values also get cleared in Redis::load(). Also added its tests.

Tell me if anything else should be done!

[ackintosh]

I've confirmed that the adapter works fine!

[ackintosh]

v1.2.3, contains this fix has been released. 🎉

@ariasmn Thank you again for your great work! ✨

[ackintosh]

@ariasmn Btw, may I add your company/project as a user in the README? If it's okay with you, please let me know. 😃

[ariasmn]

@ariasmn Btw, may I add your company/project as a user in the README? If it's okay with you, please let me know.

Sure, I asked my CTO and it'd be a pleasure for all of us! 😃

Our company is called Dapda. Here is the website.

[ackintosh]

I've add your company to the list. ✨ Thank you so much!