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

Proof-of-concept for automatic key prefixing #3770

Closed
wants to merge 5 commits into from

Conversation

killergerbah
Copy link

As per the suggestion from @sazzad16 in this discussion: #3765, I have created a POC that implements automatic key prefixing in the hopes of discussing possible changes that would allow applications to do this.

This POC demonstrates that key-prefixing becomes possible when

  • UnifiedJedis can be supplied with a custom CommandObjects. Some of the constructors that allow this would need to be made public.
  • Further changes would be necessary for CommandObjects to not call the overridden commandArguments before the child class constructor finishes, which would allow the prefix to become an instance field.

@sazzad16
Copy link
Collaborator

@killergerbah Thank you for your POC.

  • This PR considers only cluster mode. Some idea showing non-cluster mode would also be helpful.
  • It introduces a new client class JedisClusterWithPrefixedKeys which, IMO, should not be the way to go. One approach could be using config/parameter.
  • Manipulating CommandArguments and CommandObjects is an interesting and good idea and can be investigated further.

@killergerbah
Copy link
Author

killergerbah commented Mar 14, 2024

@sazzad16 Thanks for taking a look.

I have extended the POC to demonstrate an approach that would work for all subclasses of UnifiedJedis. I believe using this approach the only changes required would be to expose constructors that allow customization of CommandObjects. The sample *WithPrefixedKeys classes demonstrate that this is possible.

As mentioned in the commit message, I don't think it's necessary for jedis to provide any of the key-prefixing behavior itself. As long as the constructors are available then applications can customize the behavior of UnifiedJedis as necessary.

It might be worth noting that there is some unavoidable duplicated code in the POC. I believe this is due to CommandArguments being a concrete class and not an interface that can be composed with.

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

I like this idea.
It is now easier to think.

  1. Move the new source classes in redis.clients.jedis.util.prefix package.
  2. Move the new test classes in redis.clients.jedis.prefix package.

src/main/java/redis/clients/jedis/CommandObjects.java Outdated Show resolved Hide resolved
src/test/java/redis/clients/jedis/PrefixedKeysTest.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/JedisSentineled.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/JedisPooled.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/JedisCluster.java Outdated Show resolved Hide resolved
@killergerbah
Copy link
Author

Thanks @sazzad16 for reviewing.

There is one idea that just came to mind that may be worth considering.
Since it seems like you are willing for jedis to support key prefixing as an out-of-the-box behavior, I wonder if it would make sense to make sense to expose this behavior as an additional prefix field in JedisClientConfig. It seems that that would make it easier for callers to use the feature, rather than having to assemble the correct CommandObjects etc.

If that idea does not seem better, then I am happy to continue with this approach.

@killergerbah
Copy link
Author

killergerbah commented Mar 16, 2024

@sazzad16 I have made the requested changes, and additionally changes to support key-prefixing in transactions. Transaction support required adding CommandObjects.watch, and using CommandObjects for WATCH commands throughout the code.

However, I noticed that there does not seem to be a way to use WATCH with transactions using UnifiedJedis.multi() - is this intentional? Edit: Never mind, I see there is an issue for this (#3662).

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

Hmm 🤔, this is getting complex.

src/main/java/redis/clients/jedis/Transaction.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/Transaction.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/CommandObjects.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/CommandObjects.java Outdated Show resolved Hide resolved
R-J Lim added 5 commits March 18, 2024 14:26
- Demonstrated automatic key-prefixing for all subclasses of
  UnifiedJedis: JedisCluster, JedisPooled, and JedisSentineled
- Key-prefixing is possible as long as the underlying CommandObjects can
  be customized.
- CommandObjects cannot use commandArguments in its constructor since
  in the specific case of key-prefixing, commandArguments depends on the
  child constructor running first. So we lose caching of argument-less
  CommandObjects.
- Based on this POC, the minimum changes required to jedis would be:
  - public constructors that allow UnifiedJedis and its subclasses to
    take a custom CommandObjects.
  - Consistent use of supplied CommandObjects throughout code (e.g. in
    Pipeline, Transaction, etc).
  - Removal of caching of argument-less CommandObjects in the
    constructor of CommandObjects.
- Applications can then supply CommandObjects with custom behavior as
  necessary. Sample classes that implement the behavior of prefixed keys,
  etc are provided but these can be supplied by the application as long
  as required constructors are available.
- Restore cached key-less commands in CommandObjects
- Support Transactions
- New constructors do not take CommandExecutor
- Requested JavaDoc regarding new constructors specifying RedisProtocol
- New classes moved into 'prefix' packages
- De-duplicate prefixing code
- Restore public Transaction constructor that was removed
- Use Connection.executeCommand instead of Connection.sendCommand
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 85.93750% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 76.72%. Comparing base (56ef26a) to head (28eb185).
Report is 2 commits behind head on master.

❗ Current head 28eb185 differs from pull request most recent head 66f6336. Consider uploading reports for the commit 66f6336 to get more accurate results

Files Patch % Lines
...java/redis/clients/jedis/util/prefix/Prefixer.java 61.53% 3 Missing and 2 partials ⚠️
src/main/java/redis/clients/jedis/Transaction.java 77.77% 2 Missing ⚠️
...dis/clients/jedis/mcf/MultiClusterTransaction.java 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3770      +/-   ##
============================================
+ Coverage     76.69%   76.72%   +0.03%     
- Complexity     5130     5150      +20     
============================================
  Files           301      306       +5     
  Lines         15086    15135      +49     
  Branches       1134     1136       +2     
============================================
+ Hits          11570    11613      +43     
- Misses         3015     3019       +4     
- Partials        501      503       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sazzad16
Copy link
Collaborator

@killergerbah I have tried same feature a bit differently in #3781. Let me know what you think.

@killergerbah
Copy link
Author

@killergerbah I have tried same feature a bit differently in #3781. Let me know what you think.

Thank you. I like your approach because it uses composition instead of inheritance to achieve the same outcome. It also does not require modification of constructors of UnifiedJedis and all of its subclasses. From a caller point of view it is easier to use since it requires just setting the command key processor instead of picking the right constructor to use. I would be supportive of continuing with your branch.

@sazzad16 sazzad16 added this to the 5.2.0 milestone Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants