Support Redis inline commands by mseitner · Pull Request #7687 · netty/netty

mseitner · 2018-02-04T03:13:51Z

Motivation:
The RESP protocol implementation lacked inline command
support.

Modifications:
Added logic to decode and encode inline commands.

Result:
Inline commands are supported. Fixes #7686.

Motivation: The RESP protocol implementation lacked inline command support. Modifications: Added logic to decode and encode inline commands. Result: Inline commands are supported. Fixes #7686.

Motivation: There is some cleanup that can be done. Modifications: - Remove unused code - Move common code to base class Result: Cleaner code.

normanmaurer · 2018-02-05T07:57:39Z

@ddossot PTAL

ddossot

One nit, one question, otherwise LGTM 👍

ddossot · 2018-02-05T16:38:58Z

+     * Returns length of this type.
+     */
+    public int length() {
+        return (value != null) ? RedisConstants.TYPE_LENGTH : 0;


nit: remove parenthesis around condition for consistency with other ternaries, for ex. in RedisDecoder

ddossot · 2018-02-05T16:40:31Z

            return ARRAY_HEADER;
        default:
-            throw new RedisCodecException("Unknown RedisMessageType: " + value);
+            return INLINE_COMMAND;


We're changing a catch-all exception to an accept-all use case. I'm wondering if there's something more accurate we could do, for ex. only recognize a-zA-Z as the marker of an inline command and restore the RedisCodecException for all other cases?

Redis commands can be renamed for security reasons. Substitute commands are not limited to a-zA-Z though:

redis.conf

rename-command PING もしもし # hello in Japanese

Invoke renamed command (works in redis-cli, too)

$ echo もしもし | nc localhost 6379 +PONG

I don't see any good reason to be more restrictive than Redis itself is.

The intent was not to be more restrictive than Redis is. Since rename-command allows using any character in commands, there's unfortunately not much check we can do.

Scottmitch · 2018-02-06T19:20:53Z

I wondering if we even need to support these inline-commands. It seems like an alternative variant on the original protocol for text-based clients which may require additional work to represent the original protocol [1]. It also introduces additional complexity for encoding/decoding (as indicated above in #7687 (comment)) and may allow for malformed input to be parsed.

[1] https://redis.io/topics/protocol#inline-commands

Sometimes you have only telnet in your hands and you need to send a command to the Redis server

ddossot · 2018-02-06T19:44:45Z

@Scottmitch The inline protocol being human centric, I reckon the main use case would be people building new cli tools for Redis, which is a narrow use case.

@mseitner Did you have other use cases in mind?

mseitner · 2018-02-06T20:57:31Z

I'm working on a Redis proxy, so I have to be transparent towards the client. I got everything up and running, but of all things it was redis-benchmark that refused to work. Why? The first thing it does is calling ping as inline command, then as regular (bulk) command:

$ redis-benchmark -c 1 -n 1
====== PING_INLINE ======
  1 requests completed in 0.00 seconds
  1 parallel clients
  3 bytes payload
  keep alive: 1

100.00% <= 0 milliseconds
1000.00 requests per second

====== PING_BULK ======
  1 requests completed in 0.00 seconds
  1 parallel clients
  3 bytes payload
  keep alive: 1

[...]

However, my concrete use case should have nothing to do with the level of protocol support. Do we agree that inline commands are part of the RESP protocol? If so, they should be supported.

Regarding backward compatibility/consistency I suggest to make inline command support configurable via constructor argument, with the default being false. That should make both sides happy :-)

mseitner · 2018-02-08T13:41:21Z

@ddossot @Scottmitch How should I continue?

normanmaurer · 2018-02-08T15:21:03Z

@mseitner I would say as long as it is "optional" and the old behaviour is the default it should be "ok"

ddossot · 2018-02-08T15:22:14Z

@mseitner +1 for optional support for inline-cmd, off by default. This would address my main concern, which I detailed above about becoming too lax by default.

mseitner · 2018-02-09T07:47:44Z

Done.

Scottmitch · 2018-02-12T17:08:27Z

the length() method isn't really used in a generic fashion here and is a bit misleading. For example we always read 1 byte above without considering length(). Can we instead handle this INLINE_COMMAND logic as a continuation of the special case above?

I'll move the entire type detection to RedisMessageType. This will also improve functional consistency with the suggested serialization change below.

Scottmitch · 2018-02-12T17:11:14Z

instead of length here, we could just let RedisMessageType write itself to the buf:

enum RedisMessageType { public void serialize(ByteBuf but) { if (value != null) { buf.writeByte(value); } } }

Scottmitch · 2018-02-12T17:14:53Z

if we introduce the serialize method above, perhaps we don't have to change to Byte. We could use \0 or some "token" to represent null.

I don't see the benefits of a magic number to represent the absence(!) of a Redis type indicator.

mseitner · 2018-02-14T00:23:37Z

Changed as promised, the entire type detection is now implemented in RedisMessageType.

normanmaurer · 2018-02-14T07:47:54Z

+
+    @Override
+    public String toString() {
+        return new StringBuilder(StringUtil.simpleClassName(this))


Just use normal string concats or at least size the StringBuilder correctly ?

As you can see I just pulled up the toString() method from existing sub-classes. The implementation is similar to that of other (core) classes. Is a more complex implementation that calculates the needed size really worth it? I suggest to never call the toString() method in a productive environment anyway.

I think it worth it... that said I think we can improve it in another PR.

Great! I'd be happy to help with that :-) Anything else that should be done?

normanmaurer · 2018-02-14T07:50:28Z

-    public static RedisMessageType valueOf(byte value) {
+    public static RedisMessageType readFrom(ByteBuf in, boolean decodeInlineCommands) {
+        final int initialIndex = in.readerIndex();
+        final RedisMessageType type = valueOf(in.readByte());


just use in.getByte(in.readerIndex()) this way you will not need to reset the index

How does that help? If I use the suggested method I instead have to manually advance the reader index in all other cases to "consume" the type indicator.

never mind.. brain fart.

normanmaurer · 2018-03-27T07:46:43Z

Squashed and cherry-picked into 4.1 (c75bc1f).

Sorry for the slow turn-around

Marian Seitner added 2 commits February 4, 2018 04:05

Support Redis inline commands

5ce6dc1

Motivation: The RESP protocol implementation lacked inline command support. Modifications: Added logic to decode and encode inline commands. Result: Inline commands are supported. Fixes #7686.

Cleanup Redis module

a9f2e76

Motivation: There is some cleanup that can be done. Modifications: - Remove unused code - Move common code to base class Result: Cleaner code.

ddossot reviewed Feb 5, 2018

View reviewed changes

ddossot approved these changes Feb 9, 2018

View reviewed changes

Scottmitch requested changes Feb 12, 2018

View reviewed changes

Address comments

4b192d1

normanmaurer approved these changes Feb 14, 2018

View reviewed changes

normanmaurer closed this Mar 27, 2018

normanmaurer self-assigned this Mar 27, 2018

normanmaurer added this to the 4.1.23.Final milestone Mar 27, 2018

Uh oh!

Conversation

mseitner commented Feb 4, 2018

Uh oh!

normanmaurer commented Feb 5, 2018

Uh oh!

ddossot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scottmitch commented Feb 6, 2018

Uh oh!

ddossot commented Feb 6, 2018

Uh oh!

mseitner commented Feb 6, 2018

Uh oh!

mseitner commented Feb 8, 2018

Uh oh!

normanmaurer commented Feb 8, 2018

Uh oh!

ddossot commented Feb 8, 2018

Uh oh!

mseitner commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseitner commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Mar 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants