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

feat: add enable read operation #598

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

proost
Copy link
Contributor

@proost proost commented Aug 5, 2024

Discussion

As we discussed before, supporting replica when using bloom filter.

@rueian
Copy link
Collaborator

rueian commented Aug 5, 2024

Hi @proost,

The docker used in Github Actions seems to be updated and the docker-compose command has gone. Please rebase this PR to the latest main branch.

return nil, err
}

if parseRedisMajorVersion(info) < 7 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Users may run a redis cluster with mixed versions and a single INFO request is not enough for checking the version of each node. Actually, I think this check is unnecessary. Stating the version requirement in the comment of WithEnableReadOperation is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users may run a redis cluster with mixed versions and a single INFO request is not enough for checking the version of each node.

Oh, i missed. thank you.

8832748

rueidisprob/bloomfilter.go Outdated Show resolved Hide resolved
@proost proost requested a review from rueian August 6, 2024 15:25
@rueian rueian merged commit 3575979 into redis:main Aug 6, 2024
19 checks passed
@rueian
Copy link
Collaborator

rueian commented Aug 6, 2024

Merged. But I forgot that the parseRedisMajorVersion is still there. Could you send another PR to remove it?

@proost proost deleted the feat-enable-read-operation branch August 7, 2024 15:58
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.

2 participants