feat(server): use listpack node encoding for list #3914

adiholden · 2024-10-13T08:43:32Z

Until now encoding of list was using ziplist encoding, which is supported by redis 6
This PR introduces encoding of list using listpack encoding by default (supported by redis 7), optionally change the encoding to ziplist encoding using flag.
This PR also fixes a bug: when serializing big entry of list it was serialized as plain node using lispack encoding therefore when loading the rdb we got error. By moving encoding all the list nodes to listpack encoding we fix this bug.

adiholden · 2024-10-13T08:56:24Z

src/server/rdb_save.cc

@@ -439,7 +444,8 @@ error_code RdbSerializer::SaveListObject(const PrimeValue& pv) {
    DVLOG(3) << "QL node (encoding/container/sz): " << node->encoding << "/" << node->container
             << "/" << node->sz;

-    if (QL_NODE_IS_PLAIN(node)) {


Note: I still need to fix the way we serialize plane node when list_rdb_encode_v2=false

I talked to @chakaz about this and suggested to create an appropriate dump by redis 6 and add it to our rdb_test.

@adiholden asked me to look into adding a full redis 7 replication test, in addition to our existing redis 6 one
that should be a superset (well, almost) of what you asked for

we have replication tests with redis server which include list type, this is checking the rdb load logic in dfly
we can add there big values if needed

romange · 2024-10-13T09:13:22Z

have not checked the code but It is important that we still be able to load redis-6 rdb files and old DF snapshots with ziplist encoding unrelated to the flag.

chakaz · 2024-10-13T09:30:36Z

src/server/rdb_save.cc

@@ -439,7 +444,8 @@ error_code RdbSerializer::SaveListObject(const PrimeValue& pv) {
    DVLOG(3) << "QL node (encoding/container/sz): " << node->encoding << "/" << node->container
             << "/" << node->sz;

-    if (QL_NODE_IS_PLAIN(node)) {


@adiholden asked me to look into adding a full redis 7 replication test, in addition to our existing redis 6 one
that should be a superset (well, almost) of what you asked for

chakaz · 2024-10-13T09:33:13Z

src/server/rdb_save.cc

@@ -49,6 +49,7 @@ ABSL_FLAG(dfly::CompressionMode, compression_mode, dfly::CompressionMode::MULTI_
          "set 2 for multi entry zstd compression on df snapshot and single entry on rdb snapshot,"
          "set 3 for multi entry lz4 compression on df snapshot and single entry on rdb snapshot");
 ABSL_FLAG(int, compression_level, 2, "The compression level to use on zstd/lz4 compression");
+ABSL_FLAG(bool, list_rdb_encode_v2, true, "Use v2 rdb encoding of list object");


can we run the list unit tests once with this true and once with false?
also, didn't you say you wanted the default to be false for this? (I could have misunderstood you, it's find to be true if you want it to be such)

also^2: I thought you wanted this flag to be toggleable at runtime (i.e. via CONFIG), is it not the case?

I do not think we need to toggle it - our cloud service does not require redis-6 RDB compatibility.

see config_registry.RegisterMutable in server_family you can set the flag in runtime
I wanted the defualt to be true, I dont see a reason to convert listpack to ziplist encoding

to run the unit test with flag false I need need first to fix the issue with encoding of big list entry with the ziplist encoding

chakaz · 2024-10-13T09:34:31Z

tests/dragonfly/snapshot_test.py

+        **BASIC_ARGS,
+        "proactor_threads": 4,
+        "dbfilename": "test-redis-load-rdb",
+        "list_rdb_encode_v2": "false",


Suggested change

"list_rdb_encode_v2": "false",

"list_rdb_encode_v2": "false", # Needed for compatibility with Redis 6

chakaz · 2024-10-13T09:36:44Z

have not checked the code but It is important that we still be able to load redis-6 rdb files and old DF snapshots with ziplist encoding unrelated to the flag.

This is indeed what @adiholden did 🤓

adiholden · 2024-10-13T09:38:08Z

have not checked the code but It is important that we still be able to load redis-6 rdb files and old DF snapshots with ziplist encoding unrelated to the flag.

@romange This PR only changes the default way we encode lists

romange · 2024-10-13T09:45:17Z

Please verify that RDB files that are created by DF with large list entries can be loaded by Redis 7 when this flag is on.
(I assume due to the bug, Redis 6 won't be able to load the rdb with this flag off).

adiholden · 2024-10-13T12:20:39Z

Please verify that RDB files that are created by DF with large list entries can be loaded by Redis 7 when this flag is on. (I assume due to the bug, Redis 6 won't be able to load the rdb with this flag off).

Shahar run the test_redis_load_snapshot with redis 7 and with the flag list_rdb_encode_v2 set to true and the test passed.
Now the bug is fixed and redis 6 can load the rdb file with big list entry when the flag is set to false, the tests test_redis_load_snapshot covers this

romange · 2024-10-13T13:06:32Z

src/server/rdb_load.cc

    if (container == QUICKLIST_NODE_CONTAINER_PLAIN) {
-      quicklistAppendPlainNode(ql, (uint8_t*)sv.data(), sv.size());
+      lp = (uint8_t*)zmalloc(sv.size());


I think that Roman is implying that this is a nasty bug :)

romange · 2024-10-13T13:08:05Z

src/server/rdb_save.cc

@@ -49,6 +49,7 @@ ABSL_FLAG(dfly::CompressionMode, compression_mode, dfly::CompressionMode::MULTI_
          "set 2 for multi entry zstd compression on df snapshot and single entry on rdb snapshot,"
          "set 3 for multi entry lz4 compression on df snapshot and single entry on rdb snapshot");
 ABSL_FLAG(int, compression_level, 2, "The compression level to use on zstd/lz4 compression");
+ABSL_FLAG(bool, list_rdb_encode_v2, true, "Use v2 rdb encoding of list object");


Please improve help string of the flag. whats is v2 rdb encoding? when the flag is true what we are compatible with?

romange · 2024-10-13T13:09:19Z

src/server/server_family.cc

@@ -861,6 +861,7 @@ void ServerFamily::Init(util::AcceptServer* acceptor, std::vector<facade::Listen
  config_registry.RegisterMutable("tls_ca_cert_dir");
  config_registry.RegisterMutable("replica_priority");
  config_registry.RegisterMutable("lua_undeclared_keys_shas");
+  config_registry.RegisterMutable("list_rdb_encode_v2");


no need, imho

if we have any prod issues with the new impl we will be able to recover

romange · 2024-10-13T13:10:06Z

tests/dragonfly/snapshot_test.py

@@ -150,7 +150,14 @@ async def test_dbfilenames(


 @pytest.mark.asyncio
-@dfly_args({**BASIC_ARGS, "proactor_threads": 4, "dbfilename": "test-redis-load-rdb"})
+@dfly_args(


why not in rdb_test.cc ?

this is a python test that checks that redis can load dragonfly snapshot. what do you want to put in rdb_test.cc?

romange

lgtm

chakaz · 2024-10-14T08:38:04Z

src/server/generic_family_test.cc

+ABSL_DECLARE_FLAG(bool, list_rdb_encode_v2);
+


If you prefer, you could also use SetTestFlag("list_rdb_encode_v2", "false"); and not declare this flag
if doesn't really matter in any way, so your call

chakaz · 2024-10-14T08:38:21Z

src/server/rdb_load.cc

    if (container == QUICKLIST_NODE_CONTAINER_PLAIN) {
-      quicklistAppendPlainNode(ql, (uint8_t*)sv.data(), sv.size());
+      lp = (uint8_t*)zmalloc(sv.size());


I think that Roman is implying that this is a nasty bug :)

Signed-off-by: adi_holden <[email protected]>

adiholden requested review from romange and chakaz October 13, 2024 08:43

adiholden commented Oct 13, 2024

View reviewed changes

chakaz previously approved these changes Oct 13, 2024

View reviewed changes

adiholden dismissed chakaz’s stale review via 8d34134 October 13, 2024 10:22

romange reviewed Oct 13, 2024

View reviewed changes

chakaz approved these changes Oct 14, 2024

View reviewed changes

adiholden added 6 commits October 15, 2024 12:42

feat server: use listpack node encoding for list

ead4a2b

Signed-off-by: adi_holden <[email protected]>

fix ziplist encode for big list item

8f90139

Signed-off-by: adi_holden <[email protected]>

flip flag

860658e

Signed-off-by: adi_holden <[email protected]>

fix unit test

3f03dee

Signed-off-by: adi_holden <[email protected]>

add comment

75d72d0

Signed-off-by: adi_holden <[email protected]>

add comment

644cb14

Signed-off-by: adi_holden <[email protected]>

adiholden force-pushed the list_v2_rdb_encoding branch from 29d8430 to 644cb14 Compare October 15, 2024 09:44

adiholden merged commit a1830e1 into main Oct 15, 2024
12 checks passed

adiholden deleted the list_v2_rdb_encoding branch October 15, 2024 10:55

adiholden mentioned this pull request Oct 22, 2024

test: add test to test big collections or collections with big values #3959

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): use listpack node encoding for list #3914

feat(server): use listpack node encoding for list #3914

adiholden commented Oct 13, 2024

adiholden Oct 13, 2024

romange Oct 13, 2024

chakaz Oct 13, 2024

adiholden Oct 13, 2024

romange commented Oct 13, 2024

chakaz Oct 13, 2024

chakaz Oct 13, 2024

chakaz Oct 13, 2024

romange Oct 13, 2024

adiholden Oct 13, 2024

adiholden Oct 13, 2024

chakaz Oct 13, 2024

chakaz commented Oct 13, 2024

adiholden commented Oct 13, 2024

romange commented Oct 13, 2024

adiholden commented Oct 13, 2024

romange Oct 13, 2024

adiholden Oct 14, 2024

chakaz Oct 14, 2024

romange Oct 13, 2024

romange Oct 13, 2024

adiholden Oct 14, 2024

romange Oct 13, 2024

adiholden Oct 14, 2024

romange left a comment

chakaz Oct 14, 2024

chakaz Oct 14, 2024

	"list_rdb_encode_v2": "false",
	"list_rdb_encode_v2": "false", # Needed for compatibility with Redis 6

		ABSL_DECLARE_FLAG(bool, list_rdb_encode_v2);

feat(server): use listpack node encoding for list #3914

feat(server): use listpack node encoding for list #3914

Conversation

adiholden commented Oct 13, 2024

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

romange commented Oct 13, 2024

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

chakaz commented Oct 13, 2024

adiholden commented Oct 13, 2024

romange commented Oct 13, 2024

adiholden commented Oct 13, 2024

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

romange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment