Upgrade go-redis/redis to version 8 #298

strobus · 2021-07-20T13:11:13Z

This PR upgrade the go-redis/redis/v7 dependency to go-redis/redis/v8. The major change between the two versions is that all methods that interact with the redis server have a context.Context parameter added. These context parameters have been surfaced up to the exported asynq interfaces.

Edit: resolves issue #293

strobus · 2021-07-20T13:14:36Z

TODO: I tested with a local redis docker container and all was well. However my redis cluster is in AWS (Elasticache). There were several test failures around date/time stamps being slightly off or task scores being off by one or two. My guess is these are due to a test design which requires a specific setup, probably locally. @hibiken do you have a recommended cluster setup for testing locally?

codecov · 2021-07-20T13:28:15Z

Codecov Report

Merging #298 (4488a5d) into master (05534c6) will not change coverage.
The diff coverage is 70.49%.

@@           Coverage Diff           @@
##           master     #298   +/-   ##
=======================================
  Coverage   73.25%   73.25%           
=======================================
  Files          20       20           
  Lines        2632     2632           
=======================================
  Hits         1928     1928           
  Misses        496      496           
  Partials      208      208

Impacted Files	Coverage Δ
asynq.go	`70.07% <ø> (ø)`
client.go	`94.06% <ø> (ø)`
inspector.go	`57.42% <ø> (ø)`
internal/base/base.go	`69.09% <ø> (ø)`
scheduler.go	`84.42% <ø> (ø)`
server.go	`85.41% <ø> (ø)`
subscriber.go	`100.00% <ø> (ø)`
internal/rdb/rdb.go	`69.83% <36.36%> (ø)`
internal/rdb/inspect.go	`68.15% <89.74%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05534c6...4488a5d. Read the comment docs.

hibiken · 2021-07-21T00:02:49Z

client.go

@@ -265,7 +266,7 @@ func (c *Client) Close() error {
 // By deafult, max retry is set to 25 and timeout is set to 30 minutes.
 //
 // If no ProcessAt or ProcessIn options are provided, the task will be pending immediately.
-func (c *Client) Enqueue(task *Task, opts ...Option) (*TaskInfo, error) {
+func (c *Client) Enqueue(ctx context.Context, task *Task, opts ...Option) (*TaskInfo, error) {


I'm not against it, but I'm not sure if we want to make this API change in this PR.
Unless you have a strong argument for this, I'll make internal changes without changing the public API of the package.

hibiken · 2021-07-21T00:05:49Z

internal/base/base.go

+	Ping(context.Context) error
+	Enqueue(ctx context.Context, msg *TaskMessage) error
+	EnqueueUnique(ctx context.Context, msg *TaskMessage, ttl time.Duration) error
+	Dequeue(ctx context.Context, qnames ...string) (*TaskMessage, time.Time, error)
+	Done(ctx context.Context, msg *TaskMessage) error
+	Requeue(ctx context.Context, msg *TaskMessage) error
+	Schedule(ctx context.Context, msg *TaskMessage, processAt time.Time) error
+	ScheduleUnique(ctx context.Context, msg *TaskMessage, processAt time.Time, ttl time.Duration) error
+	Retry(ctx context.Context, msg *TaskMessage, processAt time.Time, errMsg string) error
+	Archive(ctx context.Context, msg *TaskMessage, errMsg string) error
+	ForwardIfReady(ctx context.Context, qnames ...string) error
+	ListDeadlineExceeded(ctx context.Context, deadline time.Time, qnames ...string) ([]*TaskMessage, error)
+	WriteServerState(ctx context.Context, info *ServerInfo, workers []*WorkerInfo, ttl time.Duration) error
+	ClearServerState(ctx context.Context, host string, pid int, serverID string) error
+	CancelationPubSub(ctx context.Context) (*redis.PubSub, error) // TODO: Need to decouple from redis to support other brokers
+	PublishCancelation(ctx context.Context, id string) error


I don't think we need to change the interface Broker unless we need to.

hibiken · 2021-07-21T00:07:34Z

scheduler.go

 	if err != nil {
 		j.logger.Errorf("scheduler could not record enqueue event of enqueued task %+v: %v", j.task, err)
 	}
 }

 // Register registers a task to be enqueued on the given schedule specified by the cronspec.
 // It returns an ID of the newly registered entry.
-func (s *Scheduler) Register(cronspec string, task *Task, opts ...Option) (entryID string, err error) {
+func (s *Scheduler) Register(ctx context.Context, cronspec string, task *Task, opts ...Option) (entryID string, err error) {


Same here. I'd wait to see if we need to make this API change.

hibiken

Thank you for working on this!

Do you have a reason for changing public API in this PR? Do you think users of the package will use context for deadlines, cancelation, etc?

I think we can focus just on upgrading the redis library version in this PR, so that changes are only scoped to internal/rdb package and shouldn't affect rest of the codebase.
What do you think?

strobus · 2021-07-21T14:50:43Z

I'm not opposed to keeping the exported API as is. I'm not as familiar with the internal operation of the module, so wasn't sure if we can make assumptions about the context we pass to the underlying redis library. Would we just pass context.Background() then?

crossworth · 2021-07-21T16:01:27Z

Probably we should just pass context.Background() and if one day we need a context version we could do the approach that database/sql uses, create a EnqueueContext and RegisterContext or with options client.Enqueue(task, asynq.Context(ctx)).

see:

strobus · 2021-07-21T19:22:42Z

Yes I like those approaches, which we can add down the road. I'll back out the exported API changes in the next couple days and switch to using context.Background() internally.

This reverts commit 78e23f1.

This reverts commit afd3506.

hibiken

@strobus thank you for working one this.
One last change request:
I prefer to avoid creating a globally scoped variable ctx (both in tests and application code).
Would you mind changing it so that the scope of the variable is limited to a method or a redis command invocation? (you can just call context.Background() or context.TODO() at the call site)

strobus · 2021-08-20T00:53:12Z

I am happy to do it. This project is a crucial component in what I'm working on, so thank you for developing this.

hibiken

Sorry for going back and forth so many times (hopefully this is the last time!)

I added two comments. please take a look!
Also, it looks like this branch needs to be rebased on master.

internal/asynqtest/asynqtest.go

tools/go.mod

hibiken · 2021-08-27T21:45:57Z

Looks like conflicts still need to be resolved in internal/rdb/inspect.go.

strobus · 2021-09-02T03:41:49Z

Ah yes, missed that.

hibiken · 2021-09-02T12:55:37Z

@strobus Thank you for the hard work and patience to address all the comments 🎉 Merging the PR now.

upgrade go-redis/redis to version 8

afd3506

added loglevel flag back; inadvertently deleted

78e23f1

hibiken reviewed Jul 21, 2021

View reviewed changes

hibiken requested changes Jul 21, 2021

View reviewed changes

strobus added 3 commits August 8, 2021 15:04

Revert "added loglevel flag back; inadvertently deleted"

c2552b2

This reverts commit 78e23f1.

Revert "upgrade go-redis/redis to version 8"

3433315

This reverts commit afd3506.

upgrade go-redis/redis to v8

0f32aca

hibiken requested changes Aug 9, 2021

View reviewed changes

use new context at call point instead of global context variable

626e626

hibiken requested changes Aug 23, 2021

View reviewed changes

internal/asynqtest/asynqtest.go Outdated Show resolved Hide resolved

tools/go.mod Show resolved Hide resolved

rename ctx2 to ctx

39a9689

merge from master

4488a5d

hibiken merged commit b3ef9e9 into hibiken:master Sep 2, 2021

hibiken mentioned this pull request Sep 2, 2021

[FEATURE REQUEST] Instrumentation/Tracing for asynq redis client #321

Closed

crossworth mentioned this pull request Sep 24, 2021

[BUG] github.com/hibiken/asynq/tools v0.0.0-20210902125602-b3ef9e91a9ce build fails #325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade go-redis/redis to version 8 #298

Upgrade go-redis/redis to version 8 #298

strobus commented Jul 20, 2021 •

edited

Loading

strobus commented Jul 20, 2021

codecov bot commented Jul 20, 2021 •

edited

Loading

hibiken Jul 21, 2021

hibiken Jul 21, 2021

hibiken Jul 21, 2021

hibiken left a comment

strobus commented Jul 21, 2021

crossworth commented Jul 21, 2021 •

edited

Loading

strobus commented Jul 21, 2021

hibiken left a comment

strobus commented Aug 20, 2021

hibiken left a comment

hibiken commented Aug 27, 2021

strobus commented Sep 2, 2021

hibiken commented Sep 2, 2021

Upgrade go-redis/redis to version 8 #298

Upgrade go-redis/redis to version 8 #298

Conversation

strobus commented Jul 20, 2021 • edited Loading

strobus commented Jul 20, 2021

codecov bot commented Jul 20, 2021 • edited Loading

Codecov Report

hibiken Jul 21, 2021

Choose a reason for hiding this comment

hibiken Jul 21, 2021

Choose a reason for hiding this comment

hibiken Jul 21, 2021

Choose a reason for hiding this comment

hibiken left a comment

Choose a reason for hiding this comment

strobus commented Jul 21, 2021

crossworth commented Jul 21, 2021 • edited Loading

strobus commented Jul 21, 2021

hibiken left a comment

Choose a reason for hiding this comment

strobus commented Aug 20, 2021

hibiken left a comment

Choose a reason for hiding this comment

hibiken commented Aug 27, 2021

strobus commented Sep 2, 2021

hibiken commented Sep 2, 2021

strobus commented Jul 20, 2021 •

edited

Loading

codecov bot commented Jul 20, 2021 •

edited

Loading

crossworth commented Jul 21, 2021 •

edited

Loading