-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support EVALSHA #21
Support EVALSHA #21
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
- Coverage 85.61% 83.03% -2.58%
==========================================
Files 3 3
Lines 424 395 -29
==========================================
- Hits 363 328 -35
- Misses 45 46 +1
- Partials 16 21 +5 ☔ View full report in Codecov by Sentry. |
@yedf2 please help review |
please take a look at the Codecov report |
script.go
Outdated
import "github.com/redis/go-redis/v9" | ||
|
||
var ( | ||
deleteScript = redis.NewScript(`redis.call('HSET', KEYS[1], 'lockUntil', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a new line at the first of the script? That may make the code more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Working on this |
Coverage fixed |
batch.go
Outdated
@@ -192,10 +164,25 @@ func (c *Client) weakFetchBatch(ctx context.Context, keys []string, expire time. | |||
go func(i int) { | |||
defer wg.Done() | |||
r, err := c.luaGet(ctx, keys[i], owner) | |||
ticker := time.NewTimer(c.Options.LockSleep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put ticker inside the for loop? Code can be cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reuse & reduce allocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance gained in this way should be very very little and can be ignored. Why not use the cleaner way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplified
EVALSHA
to reduce network & cpu overhead ofEVAL
.time.Sleep
for failfast on context cancelled.