Skip to content

Conversation

@uglide
Copy link
Contributor

@uglide uglide commented Oct 20, 2025

Introduces command flags to simplify command routing.

@uglide uglide requested a review from Copilot October 20, 2025 10:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a command flags feature to the Jedis library by adding a static registry that maps Redis command names to their flags. The implementation retrieves command metadata from a Redis server and automatically generates flag mappings.

  • Adds a code generator that connects to Redis servers to extract command metadata and generate flag mappings
  • Implements a static command flags registry in CommandObject.java with comprehensive command coverage
  • Provides a getFlags() method for runtime flag lookup with optimized memory usage through shared EnumSet instances

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/test/java/redis/clients/jedis/codegen/README.md Documentation for the command flags registry generator tool
src/test/java/redis/clients/jedis/codegen/CommandFlagsRegistryGenerator.java Code generator that retrieves Redis command metadata and updates the static registry
src/main/java/redis/clients/jedis/CommandObject.java Core implementation with CommandFlag enum, static registry, and getFlags() method
pom.xml Maven configuration to exclude code generators from test execution

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@github-actions
Copy link

github-actions bot commented Oct 20, 2025

Test Results

   281 files  +1    281 suites  +1   11m 41s ⏱️ -6s
10 197 tests +9  9 132 ✅  - 591  1 065 💤 +600  0 ❌ ±0 
 2 712 runs  +9  2 712 ✅ +  9      0 💤 ±  0  0 ❌ ±0 

Results for commit fdf00b5. ± Comparison against base commit 4776709.

This pull request skips 600 tests.
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetdel
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetdelBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetex
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHgetexBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHsetex
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[1] ‑ testHsetexBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetdel
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetdelBinary
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetex
redis.clients.jedis.commands.commandobjects.CommandObjectsHashCommandsTest[2] ‑ testHgetexBinary
…

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@ggivo ggivo left a comment

Choose a reason for hiding this comment

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

Looks good.

For better traceability, I would suggest placing the generated code in a dedicated file.
Additionally, wrapping command metadata in in an interface (CommandMetadataRegistry.getFlag()) and providing it as an configurabel instance within CommandObject would be beneficial. The auto-generated registry can serve as the default, but users could replace it with a custom implementation if needed — for example, dynamically loading it from the server or providing a user-defined version.

Copy link
Contributor

@atakavci atakavci left a comment

Choose a reason for hiding this comment

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

Adding the version of server - the generator running against- as part of generated content would be nice as well.


public class CommandObject<T> {

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

lets not do this and put everything into a seperate completely auto-generate class like CommandFlagRegistry or some provider class,, and use generated content at once without need of regex or any other searching/parsing. Then just consume it from CommandObject as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

uglide and others added 3 commits October 24, 2025 17:08
- Add new interface and move CommandFlag enum there
- Generate StaticCommandFlagsRegistry instead of embedding generated code into CommandObject
- Allow passing custom CommandFlagsRegistry to ClusterClientBuilder
Comment on lines 920 to 924
public EnumSet<CommandFlag> getFlags(ProtocolCommand cmd) {
byte[] raw = cmd.getRaw();
String commandName = SafeEncoder.encode(raw).toUpperCase();
return COMMAND_FLAGS_REGISTRY.getOrDefault(commandName, EMPTY_FLAGS);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not seem to work for subcommands like "CLUSTER INFO", "CONFIG GET" ...
In this case, ProtocolCommand is actually only "CLUSTER"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this problem. See my latest commit ff09e22

@ggivo
Copy link
Collaborator

ggivo commented Nov 5, 2025

@uglide
I suggest also separating generated code (populating the StaticRegistry) from actual implmenattion so it is easier for maintaining in future
You can check PR with proposal against your branch here : : #4345

ggivo and others added 3 commits November 5, 2025 09:56
…rom actual implementation (#4345)

* (clean up) Extract generated registry initialization

* regenerate after clean up

* reformat
@uglide uglide merged commit dea46c5 into master Nov 5, 2025
15 checks passed
@uglide uglide deleted the im/introduce-command-flags branch November 5, 2025 10:09
Copy link
Collaborator

@ggivo ggivo left a comment

Choose a reason for hiding this comment

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

LGTM

@ggivo ggivo changed the title Add commands flags based on static map Adding a registry that maps command names to their flags Nov 20, 2025
@ggivo ggivo added this to the 7.1.0 milestone Nov 20, 2025
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.

4 participants