Closed akostadinov closed 2 months ago
On the other hand, https://github.com/3scale/apisonator/blob/a33b79ab60b2ee465ec3235152609da73cc34558/spec/integration/worker_async_spec.rb can be simplified after these changes. I had to refactor it in a way different threads get different redis clients, but that's not required anymore.
I think we should revert this last commit because it complicates the code, requires upgrading redis-rb for the sync mode and apparently will only make a difference when the client uses Redis 7+.
* doesn't require Redis 7, but on 7 we can be more efficient
OK.
* doesn't require upgrading redis-rb because sync client doesn't use any of the improvements
But the job fetcher is shared between the sync and async clients, how does it not use the improvements?
doesn't require upgrading redis-rb because sync client doesn't use any of the improvements But the job fetcher is shared between the sync and async clients, how does it not use the improvements?
the sync worker, just calls fetch without parameters and that it, suggested implementation keeps this call the same as before
I didn't do an extremely detailed review, and I didn't even try to understand the updates in the tests, but in general it looks good! These code updates are actually easier to understand than the previous implementation. Well done :+1:
I just had on comment on transient
param.
Use a single client and do everything within the async framework.
Do not complicate things with threads and duplicating Redis client connection.
The failing tests are redundant. Probably just have to be deleted. But wanted to first get this agreed upon and then handle the details.