add Redis OBJECT command support #650

steamboatid · 2021-08-29T08:36:33Z

Hi,
as requested at issue #649, I try to add support to OBJECT command and couple of debian-based packaging files.

…to keydb

CLAassistant · 2021-08-29T08:36:37Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

TysonAndre · 2021-08-29T13:28:40Z

src/nc_message.h

@@ -141,6 +142,7 @@ typedef enum msg_parse_result {
    ACTION( REQ_REDIS_RPUSH )                                                                       \
    ACTION( REQ_REDIS_RPUSHX )                                                                      \
    ACTION( REQ_REDIS_SADD )                   /* redis requests - sets */                          \
+    ACTION( REQ_REDIS_SADDINT )                                                                       \


SADDINT isn't merged into redis, even in the unstable branch. I'm not sure what this command is meant to be, couldn't find it in a quick google search.

EDIT: Your KeyDB proposal hasn't been reviewed yet, and regardless, there aren't currently plans to support KeyDB due to full support for KeyDB being a potentially large project

you may visit the PR at keydb

TysonAndre · 2021-08-29T13:30:31Z

src/proto/nc_redis.c

@@ -971,6 +974,11 @@ redis_parse_req(struct msg *r)
                    break;
                }

+                if (str6icmp(m, 'o', 'b', 'j', 'e', 'c', 't')) {


Just this change isn't enough to support OBJECT - twemproxy will treat the first string after the command name as the key (to shard on), but here, the subcommand is in that position, so it will go to the wrong server when there's more than one server. https://redis.io/commands/object

the main purpose for supporting OBJECT is to get the encoding sub-command. even on sharding scenario, why we can't assume that all object have same encoding?

client app must understand that the answer for other sub-command other than encoding (refcount, freq, idletime) may differ from one server to another.
even for that. why we can't support this OBJECT command?

TysonAndre · 2021-08-29T13:31:52Z

README.md

@@ -70,7 +70,7 @@ The build steps are the same (`./configure; make; sudo make install`).
      -D, --describe-stats   : print stats description and exit
      -v, --verbose=N        : set logging level (default: 5, min: 0, max: 11)
      -o, --output=S         : set logging file (default: stderr)
-      -c, --conf-file=S      : set configuration file (default: conf/nutcracker.yml)
+      -c, --conf-file=S      : set configuration file (default: /etc/nutcracker/nutcracker.yml)


This is backwards incompatible, only works on *nix-like systems, and is unrelated to new redis command support. Remove it.

noted, thanks. I will revert it.

TysonAndre · 2021-08-29T13:31:59Z

src/nc.c

@@ -28,7 +28,7 @@
 #include <nc_conf.h>
 #include <nc_signal.h>

-#define NC_CONF_PATH        "conf/nutcracker.yml"
+#define NC_CONF_PATH        "/etc/nutcracker/nutcracker.yml"


This is backwards incompatible, only works on *nix-like systems, and is unrelated to new redis command support. Remove it.

what about defining something based on OS info?

TysonAndre · 2021-08-29T13:35:01Z

debian/changelog

@@ -0,0 +1,30 @@
+nutcracker (0.4.1+dfsg-1) unstable; urgency=medium


Debian support is completely unrelated to new redis command support and should not be part of the same PR.

Additionally, I don't have any experience with debian packaging and wouldn't be able to review this, anyway - The debian package maintainers who originally wrote these files could continue to do so without these being checked in.

yes. you may ignore all the debian folder

TysonAndre · 2021-08-29T14:13:00Z

debian/patches/use_system_libyaml

@@ -0,0 +1,58 @@
+Description: Use system libyaml
+Author: Faidon Liambotis <paravoid@debian.org>


Separately from that: Though I don't work for twitter, this repo still requires the CLA for contributions, and these files are authored by someone else - https://cla-assistant.io/twitter/twemproxy

Though how that could be fixed is irrelevant since I don't plan on including the changes made for debian packagers in this repo itself for the previously stated reasons.

You represent that each of Your Contributions is Your original creation (see section 7 for submissions on behalf of others). You represent that Your Contribution submissions include complete details of any third-party license or other restriction (including, but not limited to, related patents and trademarks) of which you are personally aware and which are associated with any part of Your Contributions.

Should You wish to submit work that is not Your original creation, You may submit it to Twitter separately from any Contribution, identifying the complete details of its source and of any license or other restriction (including, but not limited to, related patents, trademarks, and license agreements) of which you are personally aware, and conspicuously marking the work as "Not a Contribution. Third-party materials licensed pursuant to: [license name(s) here]" (substituting the bracketed text with the appropriate license name(s)).

TysonAndre · 2021-08-29T14:14:37Z

debian/patches/use_system_libyaml

+ AC_MSG_RESULT($disable_stats)
+
+-# Untar the yaml-0.1.4 in contrib/ before config.status is rerun
+-AC_CONFIG_COMMANDS_PRE([tar xvfz contrib/yaml-0.1.4.tar.gz -C contrib])


Unrelatedly, these patches are outdated - twemproxy 0.5.0 is using yaml-0.2.5

Though this is irrelevant since I don't plan on including the changes made for debian packagers in this repo itself for the previously stated reasons.

sure. will change that a.s.a.p.
thanks.

new commit 2bccd1a8fcd3f7d419865b0c2797e6ce7370fc0d

TysonAndre · 2021-09-13T01:23:21Z

Debian support is completely unrelated to new redis command support and should not be part of the same PR.

Additionally, I don't have any experience with debian packaging and wouldn't be able to review this, anyway - The debian package maintainers who originally wrote these files could continue to do so without these being checked in.

Separately from that: Though I don't work for twitter, this repo still requires the CLA for contributions, and these files are authored by someone else - https://cla-assistant.io/twitter/twemproxy

Though how that could be fixed is irrelevant since I don't plan on including the changes made for debian packagers in this repo itself for the previously stated reasons.

You represent that each of Your Contributions is Your original creation (see section 7 for submissions on behalf of others). You represent that Your Contribution submissions include complete details of any third-party license or other restriction (including, but not limited to, related patents and trademarks) of which you are personally aware and which are associated with any part of Your Contributions.

Should You wish to submit work that is not Your original creation, You may submit it to Twitter separately from any Contribution, identifying the complete details of its source and of any license or other restriction (including, but not limited to, related patents, trademarks, and license agreements) of which you are personally aware, and conspicuously marking the work as "Not a Contribution. Third-party materials licensed pursuant to: [license name(s) here]" (substituting the bracketed text with the appropriate license name(s)).

I stated multiple times earlier in this review that I have no plans to add debian support to this repo.

steamboatid added 7 commits August 29, 2021 09:08

modify ditignore and merge w/ debian folder

b941a00

modify ditignore and merge w/ debian folder

79e04ea

modify ditignore and merge w/ debian folder

5c86e3e

add support to OBJECT and SADDINT redis command. SADDINT is proposed …

fb4e7c0

…to keydb

fix default config path, fix debian package config files

d74ca61

fix default config path, fix debian package config files

1aa9639

fix default config path, fix debian package config files

0c6febe

TysonAndre requested changes Aug 29, 2021

View reviewed changes

TysonAndre reviewed Aug 29, 2021

View reviewed changes

steamboatid added 2 commits August 29, 2021 23:12

fix yaml version, from yaml-0.1.4 to yaml-0.2.5

2bccd1a

fixes libyaml, using system library. add some rules at dh clean

bd4cad7

TysonAndre closed this Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Redis OBJECT command support #650

add Redis OBJECT command support #650

steamboatid commented Aug 29, 2021

CLAassistant commented Aug 29, 2021 •

edited

TysonAndre Aug 29, 2021 •

edited

steamboatid Aug 29, 2021

TysonAndre Aug 29, 2021

steamboatid Aug 29, 2021

TysonAndre Aug 29, 2021

steamboatid Aug 29, 2021

TysonAndre Aug 29, 2021

steamboatid Aug 29, 2021

TysonAndre Aug 29, 2021

steamboatid Aug 29, 2021

TysonAndre Aug 29, 2021

TysonAndre Aug 29, 2021

steamboatid Aug 29, 2021

steamboatid Aug 29, 2021

TysonAndre commented Sep 13, 2021

		@@ -0,0 +1,30 @@
		nutcracker (0.4.1+dfsg-1) unstable; urgency=medium

		@@ -0,0 +1,58 @@
		Description: Use system libyaml
		Author: Faidon Liambotis <paravoid@debian.org>

add Redis OBJECT command support #650

add Redis OBJECT command support #650

Conversation

steamboatid commented Aug 29, 2021

CLAassistant commented Aug 29, 2021 • edited

TysonAndre Aug 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TysonAndre commented Sep 13, 2021

CLAassistant commented Aug 29, 2021 •

edited

TysonAndre Aug 29, 2021 •

edited