@shameersheik _updateTriesFailures checks complicate the limiter.
I believe the limiter code shouldn't count failed tries on update.

Could you read a discussion in this PR #315 ?
There was a suggestion to add timeoutMs option inside the limiter.
It is similar to counting failed updates.
If you want to re-try update it can be configured on the level of Redis client.
Alternatively, a new wrapper can be developed. It would be much more flexible.

@shameersheik Could you explain how do you use this Lua script?
It doesn't look like non-atomic increment. It is the same atomic.

In the _upsert method , i donot wait for the response.

I get the keys using get method, calculate the response as if lua script is executed send it back
Trigger the lua script method through
upsertAndUpdateUpsertAttempts before but dont wait for it to get back

Why do you need this Lua script?
Non-atomic increment should be simple as this:

1. get points
2. newPoints = points + 1
3. set new Points

The whole script is

"const incrTtlLuaScript = redis.call('set', KEYS[1], 0, 'EX', ARGV[2], 'NX') \ local consumed = redis.call('incrby', KEYS[1], ARGV[1]) \ local ttl = redis.call('pttl', KEYS[1]) \ if ttl == -1 then \ redis.call('expire', KEYS[1], ARGV[2]) \ ttl = 1000 * ARGV[2] \ end \ return {consumed, ttl} \ ;"

other than just incrmenting new points, the script also checks and sets the ttl as well.

It is the same script that RateLImiterRedis.js uses.

Having this luascript will make the write operations (increment consumed and setting ttl) atomic.

In Summary :

1. Get and Write calls are non atomic to each other because of this. Here upsert doesnot wait for write calls to finish.
2. All the different writer operations (increment consumed and ttl setting) are atomic to each other because of the lua script.

@shameersheik Thank you for explanation.
Non-atomic increments have advantages in comparison to atomic. First of all, they are faster. RateLimiterRedisNonAtomic should be truly non-atomic, otherwise there is no point in having a new rate limiter type. Developer can call get method from any limiter already.

How to implement non-atomic increments? When you get points from Redis, you can get ttl as well. Based on that ttl you can reset the value or increment it and set it back. You don't need anything except get, pttl and set methods to do non-atomic increments. You don't need Lua script. It should be simple and clear code.

The implementation of rate limiter in this PR is not ideal. It makes more get requests and then another atomic increment with Lua script. Could you rewrite it?

If you need to make configurable attempts you can create a wrapper for that. No need to complicate limiter code.

The original problem :
#222

It is not possible to horizontally scale due to using lua script's response as a read command in RedisRateLimiterRedis class.

Redis takes Lua script as a write command and hits the master node always not using the read nodes at all. This causes two problems.

1. Increased latency - all commands go to a single node while other nodes can take up the work.
2. Availability - if the master node is over utilized we cannot horizontally scale, only option would be to pay and increase the node memory. We cannot autoscale.

Problem in this thread :

Go one more step to split the ttl and the value increment commands as well to make them non atomic as well. To my knowledge, the redis keeps track of the increments(value) in an amount of time (ttl),

1. if we split them further, what purpose would it solve ? , if i hit monitor log on the redis server, we will never even be able to figure out, which increments were even done in which timerange. - I would say this would cause confusion when debugging. From my experience, this was how I was able to figure out this issue Lua script takes non integer argument as input which causes random error on user supplied input #221
2. We split the get value and set value, ttl commands from the lua script because it is advantageous to send get and set commands to different nodes (read and write). I donot see an use in splitting two set(write) commands since both of them would still go one by one (increasing network latency) to redis.

Do we want to rewrite this class name to something else other than saying it as non atomic. Let me know what you think.

@shameersheik

if we split them further, what purpose would it solve

I don't suggest to split get and ttl commands. I suggest to use already implemented _get method, check it here. https://github.com/animir/node-rate-limiter-flexible/blob/master/lib/RateLimiterRedis.js#L152 It returns value and ttl in one multi call like this.

this.client
 .multi()
 .get(rlKey)
 .pttl(rlKey)

It perfectly works in Redis Cluster.
After you get the value you can increment it in Node.js and set a new value with set command. That's it.

I donot see an use in splitting two set(write) commands since both of them would still go one by one (increasing network latency) to redis

You don't need to do two set requests. One is enough.

So instead of calling the lua script after get here, we call the put command directly ?

Exactly.
non-atomic upsert should be simple as: get value, newValue = value + 1, set newValue.