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

Improve Codec API #118

Closed
mp911de opened this issue Aug 17, 2015 · 3 comments
Closed

Improve Codec API #118

mp911de opened this issue Aug 17, 2015 · 3 comments
Labels
type: feature A new feature
Milestone

Comments

@mp911de
Copy link
Collaborator

mp911de commented Aug 17, 2015

The Codec API is based on RedisCodec which defines methods to encode and decode keys and values. The class has following issues:

  • asymmetric methods: encode returns byte[] whereas decode accepts ByteBuffer
  • The class is abstract, but could be easily an interface (raised by @prepor)
  • Users have to create a byte[]. This is not explicitly necessary. All encoded values are written into a nio ByteBuffer, so only a source for bytes is needed (raised by @beckje01)

Constraints of the improved API:

  • Break as little as possible and preserve RedisCodec as much as possible
  • Change the connect() methods in a compatible way

Proposed change:

  1. Introduce a new abstraction of RedisCodec that returns ByteBuffer on encode Another abstraction requires to change all API's where RedisCodec is referenced. Plan B: Turn RedisCodec into an interface
  2. Adopt existing codecs
  3. Explain migration in the 3.x to 4.x migration guide

Why ByteBuffer? ByteBuffers are handles to memory and encapsulate data. Netty's ByteBufs work internally with ByteBuffers and can produce references to the data without actually duplicating it. ByteBuffer is used already in RedisCodec.

@mp911de mp911de added type: feature A new feature status: help-wanted An issue that a contributor can help us with for: team-attention An issue we need to discuss as a team to make progress labels Aug 17, 2015
mp911de added a commit that referenced this issue Aug 25, 2015
@mp911de
Copy link
Collaborator Author

mp911de commented Aug 25, 2015

After implementing the code, I'm not sure whether it's worth the change.

@beckje01
Copy link

I think it looks good and eliminates the byte duplication that was required with the byte[] at the codec stage. Also all for switching it to an interface, looks nice and clean.

mp911de added a commit that referenced this issue Aug 30, 2015
@mp911de
Copy link
Collaborator Author

mp911de commented Aug 30, 2015

Implemented for 4.0

@mp911de mp911de closed this as completed Aug 30, 2015
@mp911de mp911de added this to the Lettuce 4.0 milestone Aug 30, 2015
@mp911de mp911de removed for: team-attention An issue we need to discuss as a team to make progress status: help-wanted An issue that a contributor can help us with labels Aug 30, 2015
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