Skip to content
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

Merged
merged 8 commits into from
Jan 29, 2024
Merged

Support EVALSHA #21

merged 8 commits into from
Jan 29, 2024

Conversation

WqyJh
Copy link
Contributor

@WqyJh WqyJh commented Jan 17, 2024

  • Support EVALSHA to reduce network & cpu overhead of EVAL.
  • Support cancellable sleep instead of time.Sleep for failfast on context cancelled.

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (3bdca0f) 85.61% compared to head (b20aab9) 83.03%.
Report is 2 commits behind head on main.

❗ Current head b20aab9 differs from pull request most recent head 2040bb9. Consider uploading reports for the commit 2040bb9 to get more accurate results

Files Patch % Lines
batch.go 83.33% 2 Missing and 2 partials ⚠️
client.go 84.61% 2 Missing and 2 partials ⚠️
utils.go 60.00% 2 Missing and 2 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

@WqyJh
Copy link
Contributor Author

WqyJh commented Jan 25, 2024

@yedf2 please help review

@yedf2
Copy link
Contributor

yedf2 commented Jan 25, 2024

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)
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@WqyJh
Copy link
Contributor Author

WqyJh commented Jan 26, 2024

please take a look at the Codecov report

Working on this

@WqyJh
Copy link
Contributor Author

WqyJh commented Jan 26, 2024

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)
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reuse & reduce allocation

Copy link
Contributor

@yedf2 yedf2 Jan 27, 2024

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified

@yedf2 yedf2 merged commit a01baf8 into dtm-labs:main Jan 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants