feat: Added Reset to TokenBucket #90
Conversation
|
@mennanov Let me know whether the implementation and the test suite is fine. Else I will make the necessary changes |
| t.logger.Log(err) | ||
| } | ||
| }() | ||
| state, err := t.backend.State(ctx) |
There was a problem hiding this comment.
I believe you don't need you retrieve the state first and immediately override its fields.
There was a problem hiding this comment.
ohh yeah, my bad i will remove the code. Is the other stuff fine?
There was a problem hiding this comment.
Race condition occurs while using a new TokenBucketState. Without retrieving the state and using it, error occurs.
For Reference Memcache CompareAndSwap. It expects the item's key to be the same while other values may differ.
There was a problem hiding this comment.
You can skip that check if the state is zero.
diff --git a/tokenbucket.go b/tokenbucket.go
index 16fc8ec..0b499b1 100644
--- a/tokenbucket.go
+++ b/tokenbucket.go
@@ -562,7 +562,7 @@ func (t *TokenBucketMemcached) SetState(ctx context.Context, state TokenBucketSt
Value: b.Bytes(),
CasID: t.casId,
}
- if t.raceCheck && t.casId > 0 {
+ if t.raceCheck && t.casId > 0 && !state.isZero() {
err = t.cli.CompareAndSwap(item)
} else {
err = t.cli.Set(item)
There was a problem hiding this comment.
Alternatively we may consider adding a separate Reset() method to the TokenBucketStateBackend interface.
That way we won't need a special logic in the SetState() to handle a reset.
However we will need to implement it for every backend.
What do you think?
| @@ -122,6 +122,33 @@ func (t *TokenBucket) Limit(ctx context.Context) (time.Duration, error) { | |||
| return t.Take(ctx, 1) | |||
| } | |||
|
|
|||
| // Reset the bucket to zero state | |||
| func (t *TokenBucket) Reset(ctx context.Context) (time.Duration, error) { | |||
There was a problem hiding this comment.
What is the returned time.Duration value used for?
Is it needed? If so, please add a comment explaining what it means.
| t.logger.Log(err) | ||
| } | ||
| }() | ||
| state, err := t.backend.State(ctx) |
There was a problem hiding this comment.
Alternatively we may consider adding a separate Reset() method to the TokenBucketStateBackend interface.
That way we won't need a special logic in the SetState() to handle a reset.
However we will need to implement it for every backend.
What do you think?
|
@mennanov While trying to set a new state without invoking |
Motivation
Added new Reset feature to reset the state of the bucket
Fixes #19
Description
Reset allows the state of the bucket to be set to zero value.
Implementation done for