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.

Sign up for GitHub

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

Jump to bottom

Update redis/go-redis to v9 #602

Closed
wants to merge 1 commit into from
Closed

Update redis/go-redis to v9 #602

wants to merge 1 commit into from

Conversation

l0nax
Copy link
Contributor

@l0nax l0nax commented Jan 11, 2023 •
edited
Loading

Version 9.0.1 implements the support for Redis v7.

Changelog of redis/go-redis can be found here: https://github.com/redis/go-redis/blob/master/CHANGELOG.md

l0nax added a commit to fabmation-gmbh/asynq that referenced this pull request Jan 11, 2023
@l0nax
Temporary fork until PR is accepted
d1644c7 
We need to temporary fork the library until
hibiken#602 is accepted to prevent
blocking our internal development.
l0nax added a commit to fabmation-gmbh/asynq that referenced this pull request Jan 11, 2023
@l0nax
Temporary fork until PR is accepted
213353e 
We need to temporary fork the library until
hibiken#602 is accepted to prevent
blocking our internal development.
@fr3fou
Copy link

fr3fou commented Jan 21, 2023

any updates on this? :D

@khasanovbi
Copy link

Also github.com/go-redis/redis moved to github.com/redis/go-redis :)

@l0nax
Update redis/go-redis to v9
abdfea8 
Version v9 implements the support for Redis v7 and has some
other improvements.
@l0nax
Copy link
Contributor Author

l0nax commented Jan 31, 2023

@khasanovbi, thanks for pointing that out! I updated the PR accordingly.

@l0nax l0nax changed the title Update go-redis/redis to v8.11.5 Update go-redis/redis to v9 Jan 31, 2023
@l0nax l0nax changed the title Update go-redis/redis to v9 Update redis/go-redis to v9 Jan 31, 2023
@khasanovbi
Copy link

Hi @hibiken any chance that this PR would be accepted?
For me this is the last blocker to move to redis v7.

@kamikazechaser
Copy link
Collaborator

This needs to be tested for backwards compatibility with at least redis v6 (redis/go-redis#2241).

@khasanovbi
Copy link

khasanovbi commented Feb 20, 2023 •
edited
Loading

go-redis maintainer there said But github.com/go-redis/redis/v9 should also work with Redis 6.
I'm currently use v9 lib with redis v6 in production, and it's work fine (both direct redis commands calls and lua scripts).

@kamikazechaser kamikazechaser mentioned this pull request Mar 6, 2023
Closed
kamikazechaser
kamikazechaser reviewed Mar 6, 2023
View reviewed changes
x/go.mod
@@ -3,8 +3,9 @@ module github.com/hibiken/asynq/x
go 1.16

require (
github.com/go-redis/redis/v8 v8.11.4
github.com/go-redis/redis/v8 v8.11.4 // indirect
Copy link
Collaborator

Choose a reason for hiding this comment

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

v8 is still being indirectly referenced in the x folder.

Copy link
Owner

Choose a reason for hiding this comment

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

Please fix

Copy link
Contributor

Choose a reason for hiding this comment

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

@l0nax please fix.

Check module used with cmd: go mod why github.com/go-redis/redis/v8

kamikazechaser
kamikazechaser reviewed Mar 6, 2023
View reviewed changes
tools/go.mod
github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect
github.com/fsnotify/fsnotify v1.4.9 // indirect
github.com/gdamore/encoding v1.0.0 // indirect
github.com/go-redis/redis/v8 v8.11.4 // indirect
Copy link
Collaborator

Choose a reason for hiding this comment

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

v8 still being referenced in the tools folder.

Copy link
Owner

Choose a reason for hiding this comment

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

Please fix

Copy link
Contributor

Choose a reason for hiding this comment

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

@l0nax please fix.

Check module used with cmd: go mod why github.com/go-redis/redis/v8

@kamikazechaser
Copy link
Collaborator

There are also references of v8 in server_test.go:

I think these can be removed.

Otherwise looks good, hope this can land fast!

cc/ @hibiken

@trungdlp-wolffun
Copy link
Contributor

@l0nax any update for this :D

@l0nax
Copy link
Contributor Author

l0nax commented Mar 15, 2023

As far as I'm concerned, no.

If there's anything I can do, feel free to reach out to me.
From my point of view @hibiken has to merge/ review the PR.

@trungdlp-wolffun
Copy link
Contributor

@l0nax please check PR review of @kamikazechaser, I think we need completely moved to v9

@kamikazechaser
Copy link
Collaborator

kamikazechaser commented Mar 15, 2023 •
edited
Loading

I merged this PR into my fork. I'll be testing it today with redis v7. Some tests need to be changed though esp around Z (sorted set member) otherwise the tests will fail. I think there is something in x still indirectly referencing v8, maybe a test, I didn't dig deep.

@trungdlp-wolffun
Copy link
Contributor

@kamikazechaser You can check it with go mod why github.com/go-redis/redis/v8

Reference: https://stackoverflow.com/questions/68921825/how-to-determine-which-module-s-use-an-indirect-dependency

@kamikazechaser
Copy link
Collaborator

Ah ok I see, so x/metrics needs to be updated once the main module is updated. Then the indirect reference will be removed.

@trungdlp-wolffun
Copy link
Contributor

@kamikazechaser When can we merge this pull request?

@kamikazechaser
Copy link
Collaborator

Only @hibiken can.

hibiken
hibiken reviewed Mar 17, 2023
View reviewed changes
Copy link
Owner

@hibiken hibiken left a comment

Choose a reason for hiding this comment

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

Sorry for delay in reviewing this PR. Once the open comments are addressed, I'll merge this PR. Thanks!

@trungdlp-wolffun
Copy link
Contributor

Waiting to this pull request can merge 🥳

@trungdlp-wolffun trungdlp-wolffun mentioned this pull request Mar 22, 2023
Closed
@trungdlp-wolffun
Copy link
Contributor

trungdlp-wolffun commented Mar 22, 2023 •
edited
Loading

@hibiken @l0nax I already fixed at fabmation-gmbh#1, help check it
CC: @kamikazechaser

@jerroldgao
Copy link

jerroldgao commented Mar 29, 2023 •
edited
Loading

go-redis v9 add an option support ClientName, this make it better monitoring the Redis. Could you add Redis opts that allow us to add client name?

@kamikazechaser
Copy link
Collaborator

kamikazechaser commented Mar 29, 2023 •
edited
Loading

This PR is kind of stale. Maybe I re-open my PR with the test fixes? What do you think @trungdlp-wolffun ?

@trungdlp-wolffun
Copy link
Contributor

@kamikazechaser OK, I will contribute to it

@kamikazechaser kamikazechaser mentioned this pull request Mar 31, 2023
Merged
@l0nax l0nax closed this Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trungdlp-wolffun trungdlp-wolffun trungdlp-wolffun left review comments

@kamikazechaser kamikazechaser kamikazechaser left review comments

@hibiken hibiken hibiken left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@l0nax @fr3fou @khasanovbi @kamikazechaser @trungdlp-wolffun @jerroldgao @hibiken