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

Add client parameter when creating Vertx Redis client #43454

Merged
merged 1 commit into from
Oct 3, 2024
Merged

Add client parameter when creating Vertx Redis client #43454

merged 1 commit into from
Oct 3, 2024
+191 −7

Conversation

mcruzdev
Copy link
Contributor

@mcruzdev mcruzdev commented Sep 24, 2024 •
edited by geoand
Loading

cescoffier
cescoffier requested changes Sep 25, 2024
View reviewed changes
...is-client/runtime/src/main/java/io/quarkus/redis/runtime/client/VertxRedisClientFactory.java Outdated
@@ -107,6 +113,32 @@ public static Redis create(String name, Vertx vertx, RedisClientConfig config, T
return Redis.createClient(vertx, options);
}

public static String generateConnectionString(String clientName, URI uri) {
Copy link
Member

Choose a reason for hiding this comment

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

You changed the client name to URL-encodable. It may not be correct. If you have a client name for observability purposes (checking the connection on Redis, for example), your client name was arbitrarily modified.

It would be better to encode the client name correctly using URL encoding.

Copy link
Member

Choose a reason for hiding this comment

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

Another solution (easier) would be to document the transformation.

Copy link
Member

Choose a reason for hiding this comment

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

@Ladicek what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the transformation here is a big deal, but there should be some documentation indeed.

integration-tests/redis-client/src/main/java/io/quarkus/redis/it/RedisClientResource.java Outdated
private final RedisDataSource client11;

@Inject
public RedisClientResource(@RedisClientName("quarkiverse") RedisDataSource ds,
Copy link
Member

Choose a reason for hiding this comment

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

We would need to test the various characters that are getting transformed.

@mcruzdev mcruzdev force-pushed the issue-43167 branch 3 times, most recently from 28b7d77 to 9df660f Compare September 25, 2024 18:41
@mcruzdev
Copy link
Contributor Author

Hi @cescoffier, @Ladicek as mentioned in Zulip (there is no way to link the chat) I changed the way to define a client.

We only have the option to set a client name after Redis 6, as Redis version 5 does not allow configuration via connection string. Another interesting thing is that we don’t need to worry much about special characters, we skip the URL encoding.

I'm not sure if recording this call to Redis before the application starts is the best solution. I'm open to suggestions and would appreciate any recommendations.

cescoffier
cescoffier requested changes Sep 26, 2024
View reviewed changes
.../redis-client/runtime/src/main/java/io/quarkus/redis/runtime/client/RedisClientRecorder.java Outdated
String clientName = actualConfig.clientName().orElse(name);
redisClientAndApi.redis.send(Request.cmd(Command.CLIENT, new Object[] {
"SETNAME", clientName
})).subscribe()
Copy link
Member

Choose a reason for hiding this comment

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

This approach will lead to race conditions. The execution of the command is async and thus, someone using the client immediately to emit another command may race with this (and may not have the client set).

This approach would work but we would need to make sure

  1. Redis is available
  2. Every other commands are enqueued until this one terminate successfully

@mcruzdev mcruzdev force-pushed the issue-43167 branch 2 times, most recently from 12fc51f to 90c2a2a Compare September 28, 2024 03:40
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 29, 2024 •
edited
Loading

🎊 PR Preview e2c1d31 has been successfully built and deployed to https://quarkus-pr-main-43454-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@mcruzdev mcruzdev force-pushed the issue-43167 branch 2 times, most recently from 9275bf5 to ec57afa Compare September 29, 2024 11:31
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
cescoffier
cescoffier approved these changes Sep 30, 2024
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM, @Ladicek WDYT?

@mcruzdev mcruzdev force-pushed the issue-43167 branch 2 times, most recently from 541a9c8 to d4b08d0 Compare September 30, 2024 17:08
Ladicek
Ladicek approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

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

LGTM, requested just a few tiny changes.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@cescoffier
Copy link
Member

Can you squash your commits? Thanks!

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@mcruzdev
Copy link
Contributor Author

mcruzdev commented Oct 3, 2024

done @cescoffier!

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Oct 3, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit d6264ad.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Oct 3, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit d6264ad.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@cescoffier cescoffier merged commit 9af0f74 into quarkusio:main Oct 3, 2024
33 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 3, 2024
@cescoffier
Copy link
Member

Thanks!

@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladicek Ladicek Ladicek approved these changes

@cescoffier cescoffier cescoffier approved these changes

Assignees
No one assigned
Labels
area/core area/redis area/testing kind/enhancement New feature or request
Projects
None yet
Milestone
3.16.0.CR1
Development

Successfully merging this pull request may close these issues.

Redis client list command returns empty name ("name=") when i set @RedisClientName
3 participants
@mcruzdev @cescoffier @Ladicek