Proof-of-concept for automatic key prefixing#3770
Proof-of-concept for automatic key prefixing#3770killergerbah wants to merge 5 commits intoredis:masterfrom
Conversation
|
@killergerbah Thank you for your POC.
|
|
@sazzad16 Thanks for taking a look. I have extended the POC to demonstrate an approach that would work for all subclasses of As mentioned in the commit message, I don't think it's necessary for It might be worth noting that there is some unavoidable duplicated code in the POC. I believe this is due to |
d1e4110 to
84ee29d
Compare
sazzad16
left a comment
There was a problem hiding this comment.
I like this idea.
It is now easier to think.
- Move the new source classes in
redis.clients.jedis.util.prefixpackage. - Move the new test classes in
redis.clients.jedis.prefixpackage.
|
Thanks @sazzad16 for reviewing. There is one idea that just came to mind that may be worth considering. If that idea does not seem better, then I am happy to continue with this approach. |
|
@sazzad16 I have made the requested changes, and additionally changes to support key-prefixing in transactions. Transaction support required adding However, I noticed that there does not seem to be a way to use |
8527b60 to
872fcfd
Compare
sazzad16
left a comment
There was a problem hiding this comment.
Hmm 🤔, this is getting complex.
- 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
571ed52 to
66f6336
Compare
Codecov ReportAttention: Patch coverage is
❗ 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. |
|
@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 |
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
UnifiedJediscan be supplied with a customCommandObjects. Some of the constructors that allow this would need to be madepublic.CommandObjectsto not call the overriddencommandArgumentsbefore the child class constructor finishes, which would allow the prefix to become an instance field.