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

Support ByteArrayCodec only for values #1122

Closed
dmandalidis opened this issue Sep 14, 2019 · 4 comments
Closed

Support ByteArrayCodec only for values #1122

dmandalidis opened this issue Sep 14, 2019 · 4 comments
Labels
type: feature A new feature
Milestone

Comments

@dmandalidis
Copy link
Contributor

Feature Request

I think it would worth the effort to implement a mix of StringCodec and ByteArrayCodec for keys and values respectively. While someone could easily use the ByteArrayCodec and convert the key to string afterwards, this is not possible when the ByteArrayCodec is wrapped inside the CompressionCodec. In that way, one cannot escape from having compressed keys even though the original intention would commonly (IMHO) be to just compress the value.

@mp911de I could give it a try if you feel so. My gut feeling is that, if accepted, a little bit of refactoring would be needed to avoid code duplication between the new one, StringCodec and ByteArrayCodec, but we can have a first version in a PR and discuss on how you 'd suggest to proceed

@dmandalidis dmandalidis added the type: feature A new feature label Sep 14, 2019
@mp911de
Copy link
Collaborator

mp911de commented Sep 14, 2019

Thanks for report. How if we add a codec adapter so we aid codec creation, something like RedisCodec<String, byte[]> codec = RedisCodec.of(StringCodec.UTF8, ByteArrayCodec.INSTANCE)?

@dmandalidis
Copy link
Contributor Author

Less code duplication indeed, should I proceed implementing it?

@dmandalidis
Copy link
Contributor Author

On second thought, I think I didn't understand the proposal. I guess you are referring to something like:

// RedisCodecAdapter.java
static <K, V> RedisCodec<K, V> of(RedisCodec<K, V> keyCodec, RedisCodec<K, V> valueCodec);

In that way V type for key codec is useless (like the K type in valueCodec). It seems more correct to create a RedisKeyCodec and a RedisKeyValue codec and have the adapter combine them to create a RedisCodec

@mp911de
Copy link
Collaborator

mp911de commented Sep 14, 2019

We don’t distinguish between key and value codecs. The of(...) method needs a slight tweak to genetics: key codec would be <K, ?> and value codec <?, V> as we’re combining only the portion of generics what the actual codec is used for.

Feel free to submit a PR. We’ll work out the rest over some code.

dmandalidis added a commit to dmandalidis/lettuce-core-osgi that referenced this issue Sep 14, 2019
- Supports using different codecs for keys and values
mp911de pushed a commit that referenced this issue Sep 19, 2019
- Supports using different codecs for keys and values

Original pull request: #1123.
mp911de added a commit that referenced this issue Sep 19, 2019
Rename wrapper type to ComposedRedisCodec. Introduce RedisCodec.of(…) factory method.

Add tests.

Original pull request: #1123.
@mp911de mp911de added this to the 5.2.0 milestone Sep 19, 2019
@mp911de mp911de closed this as completed Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A new feature
Projects
None yet
Development

No branches or pull requests

2 participants