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

quarkus-redis-client needs ZUNION method #18351

Closed
kafeltz opened this issue Jul 2, 2021 · 16 comments · Fixed by #22629
Closed

quarkus-redis-client needs ZUNION method #18351

kafeltz opened this issue Jul 2, 2021 · 16 comments · Fixed by #22629
Labels
area/redis kind/enhancement New feature or request
Milestone

Comments

@kafeltz
Copy link

kafeltz commented Jul 2, 2021

The ZUNION method is missing in the quarkus-redis-client. Currently there is only the ZUNIONSTORE method.

This one: https://redis.io/commands/zunion

@kafeltz kafeltz added the kind/extension-proposal Discuss and Propose new extensions label Jul 2, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 2, 2021

/cc @cescoffier, @gsmet, @machi1990

@patrox
Copy link
Contributor

patrox commented Jul 2, 2021

@kafeltz @machi1990 I can address this missing functionality if you aren't already working on this one.

@patrox
Copy link
Contributor

patrox commented Jul 2, 2021

It seems like Vert.x Redis client (which is used by quarkus-redis-client) will have to be updated as well, as it doesn't support ZUNION: https://github.com/vert-x3/vertx-redis-client/blob/master/src/main/java/io/vertx/redis/client/RedisAPI.java#L3500-L3516

@machi1990
Copy link
Member

machi1990 commented Jul 3, 2021

It seems like Vert.x Redis client (which is used by quarkus-redis-client) will have to be updated as well, as it doesn't support ZUNION: https://github.com/vert-x3/vertx-redis-client/blob/master/src/main/java/io/vertx/redis/client/RedisAPI.java#L3500-L3516

@patrox thanks for the interest in fixing the issue. Yes, the command might need to be added in upstream first in Vertx 4. @pmlopes WDYT?

As an alternative for now:

You can try using the plain vertx io.vertx.redis.client.Redis client. And use it to send the command.

redis.send(Request.cmd(Command.ZUNION).arg("arg1").arg("arg2").arg("arg3"), res -> {
  if (res.succeeded()) {
    
  }
});

using the mutiny based client: io.quarkus.redis.client.runtime.MutinyRedis will be the most appropriate

mutinyRedis.send(Request.cmd(Command.ZUNION).arg("arg1").arg("arg2").arg("arg3"))....

@patrox
Copy link
Contributor

patrox commented Jul 3, 2021

Thank you @machi1990 for the alternative suggestion, I can try that as I won't have to wait for the upstream change.

I created a feature request in Vert.x Redis Client to address this in a longer term as well:

vert-x3/vertx-redis-client#299

@machi1990
Copy link
Member

Thank you @machi1990 for the alternative suggestion, I can try that as I won't have to wait for the upstream change.

Thanks, let me know how it goes.

I created a feature request in Vert.x Redis Client to address this in a longer term as well:

vert-x3/vertx-redis-client#299

Awesome. Thank you.

@patrox
Copy link
Contributor

patrox commented Jul 5, 2021

redis.send(Request.cmd(Command.ZUNION).arg("arg1").arg("arg2").arg("arg3"), res -> {
  if (res.succeeded()) {
    
  }
});

@machi1990 I'm afraid we won't be able to proceed this way, as the Command enum doesn't include the ZUNION value :(

I updated it already on my PR which I'm going to open in vert.x redis client: https://github.com/patrox/vertx-redis-client/pull/1/files.

Maybe there should be a possibility to pass there plain Strings (i.e. "ZUNION") instead of Command values (Command.ZUNION) ?

@machi1990
Copy link
Member

redis.send(Request.cmd(Command.ZUNION).arg("arg1").arg("arg2").arg("arg3"), res -> {
  if (res.succeeded()) {
    
  }
});

@machi1990 I'm afraid we won't be able to proceed this way, as the Command enum doesn't include the ZUNION value :(

@patrox thanks for finding sometime to look at this.

Yes, we'll have to create a new command Command.create("zunion".....).

I updated it already on my PR which I'm going to open in vert.x redis client: https://github.com/patrox/vertx-redis-client/pull/1/files.

It seems that this has been fixed in vert-x3/vertx-redis-client@8d78307, we can wait for the new release and add all the new commands. It seems like there are couple others commands which might be useful. What do you think?
And as for now, we can document how the user can handcraft missing commands (see #18378 )

Maybe there should be a possibility to pass there plain Strings (i.e. "ZUNION") instead of Command values (Command.ZUNION) ?

Yes, we'll have to create a new command Command.create("zunion".....).

@patrox
Copy link
Contributor

patrox commented Jul 5, 2021

In the meantime @pmlopes has added the commands from a recent Redis server version (including ZUNION) to vert.x Redis client, so probably we will be able to use the new version soon .

@patrox
Copy link
Contributor

patrox commented Jul 5, 2021

@machi1990 Yes, I saw the other new commands which were added, so I can add these as well.

I'll monitor vert.x redis client to see if there is a new version.

I can also document the workaround method as well - is it OK to do it in a single PR ? I can create separate commits for these changes.

@machi1990
Copy link
Member

@machi1990 Yes, I saw the other new commands which were added, so I can add these as well.

I'll monitor vert.x redis client to see if there is a new version.

I can also document the workaround method as well - is it OK to do it in a single PR ? I can create separate commits for these changes.

I think what we can do is document the workaround for now in a separate PR and we'll fix the issue once a new release of the redis client is there.

@pmlopes
Copy link
Contributor

pmlopes commented Jul 5, 2021

@machi1990 @patrox the fix will arrive with vert.x 4.2.0.

The workaround required defining the zunion command, you can do it as:

public static final Command ZUNION = Command.create("zunion", -3, 0, 0, 0, false, true, true, false);

See the latest generation for 6.2.4 sources:

https://github.com/vert-x3/vertx-redis-client/blob/master/src/main/java/io/vertx/redis/client/Command.java

@patrox
Copy link
Contributor

patrox commented Jul 5, 2021

@machi1990 @patrox the fix will arrive with vert.x 4.2.0.

The workaround required defining the zunion command, you can do it as:

public static final Command ZUNION = Command.create("zunion", -3, 0, 0, 0, false, true, true, false);

See the latest generation for 6.2.4 sources:

https://github.com/vert-x3/vertx-redis-client/blob/master/src/main/java/io/vertx/redis/client/Command.java

Thanks @pmlopes, this is exactly what I was looking for!

@kafeltz
Copy link
Author

kafeltz commented Jul 8, 2021

@patrox I think the reactive library needs the zunion function too. Have you seen it?

@gastaldi gastaldi added kind/enhancement New feature or request and removed kind/extension-proposal Discuss and Propose new extensions labels Jan 4, 2022
@gastaldi
Copy link
Contributor

gastaldi commented Jan 4, 2022

Quarkus is using Vert.x 4.2.2. Is this still an issue?

@gsmet
Copy link
Member

gsmet commented Jan 4, 2022

I think we probably still need to bind the new commands in the Quarkus impl if not already done.

gastaldi added a commit to gastaldi/quarkus that referenced this issue Jan 4, 2022
gastaldi added a commit to gastaldi/quarkus that referenced this issue Jan 4, 2022
@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/redis kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants