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

Bloom support #2649

Closed
wants to merge 17 commits into from
Closed

Conversation

elena-kolevska
Copy link
Contributor

@elena-kolevska elena-kolevska commented Jul 18, 2023

part of #2068

probabilistic.go Outdated Show resolved Hide resolved
probabilistic.go Outdated Show resolved Hide resolved
probabilistic.go Outdated Show resolved Hide resolved
probabilistic.go Outdated Show resolved Hide resolved
probabilistic.go Outdated Show resolved Hide resolved
@vmihailenco
Copy link
Collaborator

You probably want to use int64/uint64 instead of int/uint everywhere, because int is either int32 or int64 depending on the architecture and I don't think that behavior is useful in the client.

@elena-kolevska
Copy link
Contributor Author

elena-kolevska commented Jul 22, 2023

The tests are not passing in CI because the code needs Redis Stack 7.2 RC3 or above. @chayim is working on a way to automate tests with a Redis Stack option.

BFReserve(ctx context.Context, key string, errorRate float64, capacity int64) *StatusCmd
BFReserveExpansion(ctx context.Context, key string, errorRate float64, capacity, expansion int64) *StatusCmd
BFReserveNonScaling(ctx context.Context, key string, errorRate float64, capacity int64) *StatusCmd
BFReserveArgs(ctx context.Context, key string, options *BFReserveOptions) *StatusCmd
Copy link
Collaborator

@vmihailenco vmihailenco Jul 25, 2023

Choose a reason for hiding this comment

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

I know that at this point the Args suffix looks like some kind of convention, but it never meant to be so and we don't really enforce it. So feel free to come with a better name :)

BFReserveExt or BFReserveOptions or BFReserveWithOptions might be it, but I don't feel strongly about it.

"github.com/redis/go-redis/v9/internal/proto"
)

type probabilisticCmdable interface {
Copy link

Choose a reason for hiding this comment

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

Why not public interface?

@elena-kolevska
Copy link
Contributor Author

I'm closing this, since the commits went in another pr that's already merged. Thanks @ofekshenawa for working on it.

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.

5 participants