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

[ISSUE #11449] Support RESP3 for codec-redis #11448

Open
wants to merge 12 commits into
base: 4.1
Choose a base branch
from

Conversation

jjz921024
Copy link
Contributor

@jjz921024 jjz921024 commented Jul 4, 2021

#11449

Motivation:

Modifications:

  • Add new types of NullRedisMessage, BooleanRedisMessage

  • Add new number types of DoubleRedisMessage, BigNumberRedisMessage

  • Add BulkErrorStringHeaderRedisMessage & FullBulkErrorStringRedisMessage for support blob error. (Just extend BulkStringHeaderRedisMessage & FullBulkStringRedisMessage)

  • Extract AbstractNumberRedisMessage to unify IntegerRedisMessage & DoubleRedisMessage & BigNumberRedisMessage

  • Modify RedisEncoder & RedisDecoder to support new types

  • Modify RedisBulkStringAggregator to support blob error message

  • Add unit tests

  • Fix some typo

  • Support encode/decode Set types redis message & Extract Collection redis message for Array types and Set types

  • Add RedisMapAggregator for support decode Map types message

  • Support Verbatim string and Push types message

  • (Todo) support Attribute, Streamed strings and Streamed aggregated data types

Result:

  • codec-redis can encode/decode RESP3

@jjz921024 jjz921024 changed the title Support RESP3 for codec-redis [ISSUE #11449] Support RESP3 for codec-redis Jul 4, 2021
@jjz921024 jjz921024 marked this pull request as ready for review July 4, 2021 11:11
@normanmaurer
Copy link
Member

Please fix check style:

[INFO] --- maven-checkstyle-plugin:3.1.0:check (check-style) @ netty-codec-redis ---
2473
[INFO] Starting audit...
2474
Error:  /home/runner/work/netty/netty/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisEncoder.java:80: Line is longer than 120 characters (found 127). [LineLength]
2475
Error:  /home/runner/work/netty/netty/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisEncoder.java:82: Line is longer than 120 characters (found 123). [LineLength]
2476
Error:  /home/runner/work/netty/netty/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisEncoder.java:88: Line is longer than 120 characters (found 124). [LineLength]
2477
Error:  /home/runner/work/netty/netty/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisDecoder.java:369: two or more consecutive empty lines [RegexpMultiline]
2478
Error:  /home/runner/work/netty/netty/codec-redis/src/main/java/io/netty/handler/codec/redis/RedisDecoder.java:381:77: Unnecessary parentheses around assignment right-hand side. [UnnecessaryParentheses]
2479
Error:  /home/runner/work/netty/netty/codec-redis/src/main/java/io/netty/handler/codec/redis/NullRedisMessage.java:23: Class NullRedisMessage should be declared as final. [FinalClass]
2480
Error:  /home/runner/work/netty/netty/codec-redis/src/main/java/io/netty/handler/codec/redis/BooleanRedisMessage.java:24: Class BooleanRedisMessage should be declared as final. [FinalClass]
2481
Error:  /home/runner/work/netty/netty/codec-redis/src/test/java/io/netty/handler/codec/redis/RedisEncoderTest.java:233: Line is longer than 120 characters (found 122). [LineLength]
2482
Audit done.
2483
[INFO] ------------------------------------------------------------------------

public abstract class AbstractCollectionRedisMessage extends AbstractReferenceCounted
implements AggregatedRedisMessage {

protected Collection<RedisMessage> children;
Copy link
Member

Choose a reason for hiding this comment

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

private final ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sub class ArrayRedisMessage and SetRedisMessage need access this children field on the abstract method children() for cast to List<Message> or Set<Message>, so it can't be private? but I had made it final

* Set of <a href="https://github.com/antirez/RESP3/blob/master/spec.md">RESP3</a>.
*/
@UnstableApi
public class SetRedisMessage extends AbstractCollectionRedisMessage {
Copy link
Member

Choose a reason for hiding this comment

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

final

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thear are a predefined EMPTY_INSTANCE of SetRedisMessage so that it can't not be final and need a default contructor.
But i not sure the EMPTY_INSTANCE is necessary?

@hyperxpro
Copy link
Contributor

Did we miss this?

@jjz921024
Copy link
Contributor Author

Did we miss this

hi, I'm still here and I fix all review. But this module will be removed out of netty?

If netty want to suppor RESP3, what can i do?

@yangbodong22011
Copy link

yangbodong22011 commented Jan 10, 2023

Will this PR be merged in 2023?

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.

None yet

4 participants