Adding basic version of DUMP and RESTORE commands #899

s3w3nofficial · 2025-01-02T03:33:06Z

Currently only work's on string type and does not support compression, but it's still a solid starting point.

Implemented

redis length encoding
redis compatible crc64 implementation
DUMP command that works on strings without compression
RESTORE command that works on strings without compression and additional modifiers

Not Implemented

support for other types than strings
support for lzf compression
optional modifiers for RESTORE

Also about the copyright for the crc64, i only ported the pycrc generated version, but still i'm not sure if and which copyright should i add.

s3w3nofficial · 2025-01-02T03:36:02Z

@microsoft-github-policy-service agree

libs/common/RedisLengthEncodingUtils.cs

badrishc · 2025-01-06T22:08:19Z

Also about the copyright for the crc64, i only ported the pycrc generated version, but still i'm not sure if and which copyright should i add.

since you only ported the pycrc generated version, it seems we should be okay as it is ...

libs/common/Crc64.cs

libs/server/Resp/Parser/RespCommand.cs

vazois · 2025-01-14T19:47:37Z

@s3w3nofficial, any updates on the open comments?

s3w3nofficial · 2025-01-15T10:49:00Z

@s3w3nofficial, any updates on the open comments?

Only open comment left is from @badrishc about the renaming which i already implemented, i guess i can close that one ?

vazois

LGTM!

libs/common/Crc64.cs

validation flag

s3w3nofficial · 2025-01-16T03:39:35Z

Every test is passing except the RestoreACLsAsync. It throws this exception: "NOAUTH Authentication required.".

I can't figure out why. Can you guys please take a look at it ?

libs/server/Resp/Parser/RespCommand.cs

libs/server/Resp/RespServerSession.cs

TalZaccai · 2025-01-16T20:05:00Z

libs/resources/RespCommandsDocs.json

@@ -2652,6 +2652,50 @@
      }
    ]
  },
+  {


Please add these commands to playground/CommandInfoUpdater/SupportedCommand.cs and run the CommandInfoUpdater tool with --force, then ensure that the produced json files (RespCommandDocs.json & RespCommandInfo.json) are the same (it might move these commands around as it's alphabetizing the commands & sub-commands). Thank you!

badrishc · 2025-01-16T20:27:30Z

Every test is passing except the RestoreACLsAsync. It throws this exception: "NOAUTH Authentication required.".

I can't figure out why. Can you guys please take a look at it ?

@kevin-montrose, can you help how to fix the ACL unit test here? The error is:

kevin-montrose · 2025-01-16T21:21:23Z

Every test is passing except the RestoreACLsAsync. It throws this exception: "NOAUTH Authentication required.".
I can't figure out why. Can you guys please take a look at it ?

@kevin-montrose, can you help how to fix the ACL unit test here? The error is:

@badrishc @s3w3nofficial I suspect it's that try/catch not leaving the client in a nice state when an exception is encountered. The pattern elsewhere for ACLs with commands that aren't repeatable is to use a counter to differentiate each invocation.

[Test]
public async Task RestoreACLsAsync()
{
    var count = 0;

    await CheckCommandsAsync(
        "RESTORE",
        [DoRestoreAsync]
    );

    async Task DoRestoreAsync(GarnetClient client)
    {
        var payload = new byte[]
        {
            0x00, // value type 
            0x03, // length of payload
            0x76, 0x61, 0x6C,       // 'v', 'a', 'l'
            0x0B, 0x00, // RDB version
            0xDB, 0x82, 0x3C, 0x30, 0x38, 0x78, 0x5A, 0x99 // Crc64
        };

        count++;

        var val = await client.ExecuteForStringResultAsync(
            "$7\r\nRESTORE\r\n"u8.ToArray(),
            [
                Encoding.UTF8.GetBytes($"foo-{count}"), 
                "0"u8.ToArray(), 
                payload
            ]);

        ClassicAssert.AreEqual("OK", val);
    }
}

Works, and all ACL tests pass.

kevin-montrose

Lots of little notes for improvements, and a couple questions around correctness.

kevin-montrose · 2025-01-16T21:22:59Z

libs/server/Resp/KeyAdminCommands.cs

+
+            // return error if key already exists 
+            var keyExists = storageApi.EXISTS(key);
+            switch (keyExists)


Why a 1 case switch instead of an if?

kevin-montrose · 2025-01-16T21:23:59Z

libs/server/Resp/KeyAdminCommands.cs

+            }
+
+            // get footer (2 bytes of rdb version + 8 bytes of crc)
+            var footer = valueSpan[^10..];


Should an error be reported if valueSpan is too small? Just raising an exception is kinda hostile.

kevin-montrose · 2025-01-16T21:27:57Z

libs/server/Resp/KeyAdminCommands.cs

+
+            if (ttl > 0)
+            {
+                storageApi.SETEX(key, valArgSlice, TimeSpan.FromMilliseconds(ttl));


What is reported if the set fails?

More generally, the Exists call on L45 won't keep the key from being created by the time we get here (or to line 107).

Perhaps a SETEX with NX (and dealing with the responses) would be a better approach? Redis has a REPLACE flag to control that behavior, but that can be future work.

kevin-montrose · 2025-01-16T21:28:13Z

libs/server/Resp/KeyAdminCommands.cs

+            }
+            else
+            {
+                storageApi.SET(key, valArgSlice);


Same note as above on SETEX.

kevin-montrose · 2025-01-16T21:28:34Z

libs/server/Resp/KeyAdminCommands.cs

+
+            var status = storageApi.GET(key, out var value);
+
+            switch (status)


Same "why switch and not if"-note.

kevin-montrose · 2025-01-16T21:30:19Z

libs/server/Resp/KeyAdminCommands.cs

+
+            // Total len (% + length of ascii bytes + CR LF + payload type + redis encoded payload len + payload len + rdb version + crc64 + CR LF)
+            var totalLength = 1 + lengthInASCIIBytesLen + 2 + 1 + encodedLength.Length + value.ReadOnlySpan.Length + 2 + 8 + 2;
+            Span<byte> buffer = stackalloc byte[totalLength];


This could be pretty big, right? So unconditionally stackalloc'ing is dangerous, probably get a rented array past a certain limit (64 or 128 are probably reasonable?).

Not sure if renting here is a good idea, if this method will get called, it's probably going to be multiple times, the renting and disposing might be overall less efficient than just heap allocating.

For now i changed it to heap alloc when above 128.

kevin-montrose · 2025-01-16T21:31:49Z

libs/server/Resp/KeyAdminCommands.cs

+            buffer[offset++] = 0x00;
+
+            // length of the span
+            foreach (var b in encodedLength)


Why copy byte-by-byte and not use a Span CopyTo method?

kevin-montrose · 2025-01-16T21:33:40Z

libs/server/Resp/KeyAdminCommands.cs

+
+            // Len of the dump (payload type + redis encoded payload len + payload len + rdb version + crc64)
+            var len = 1 + encodedLength.Length + value.ReadOnlySpan.Length + 2 + 8;
+            var lengthInASCIIBytes = new Span<byte>(new byte[NumUtils.NumDigitsInLong(len)]);


Max length here is like 10 right? You can just stackalloc that instead of allocating an array.

kevin-montrose · 2025-01-16T21:34:43Z

libs/common/RespLengthEncodingUtils.cs

+    /// <param name="buff"></param>
+    /// <returns></returns>
+    /// <exception cref="ArgumentException"></exception>
+    public static (long length, byte payloadStart) DecodeLength(ref ReadOnlySpan<byte> buff)


nit: C# convention is to PascalCase tuple members. So (long Length, byte PayloadStart).

kevin-montrose · 2025-01-16T21:36:06Z

libs/common/RespLengthEncodingUtils.cs

+    /// <param name="length"></param>
+    /// <returns></returns>
+    /// <exception cref="ArgumentOutOfRangeException"></exception>
+    public static byte[] EncodeLength(long length)


This whole method is pretty allocate-y, which worries me if it's ever extensively used.

I agree bit @kevin-montrose here. A common approach is to have small static method to first calculate the required buffer size (it can be simple switch expression) for the caller (i.e. in our case NetworkDUMP method) to know how much buffer it needs (to slice from existing buffer, stackalloc, rent etc., doesn't matter).

Then the actual encoding method takes a Span<byte> output argument which ownership belongs to caller. Now we could do the encoding on this buffer without allocations. You can see this pattern all over .NET with TryWrite and other encoding/serialization methods.

s3w3nofficial · 2025-01-16T22:22:35Z

Hey @kevin-montrose

Thanks for the help with test and for the comments. I will try to resolve these over the weekend.

Regarding the parsing, i'm working on a parser that support's the whole rdb format and i hope i will be able to implement that one in the future. For now i will look into optimizing the RespLengthEncodingUtils since the parser won't be ready any time soon.

badrishc · 2025-01-17T00:00:33Z

libs/host/defaults.conf

@@ -344,6 +344,9 @@
 	/* Fails if encounters error during AOF replay or checkpointing */
 	"FailOnRecoveryError": false,

+	/* Skips crc64 validation in restore command */
+	"SkipChecksumValidation": false,


Rename to SkipRDBRestoreChecksumValidation to clarify that this skip is specifically for the restore command (we have checksum validations in other unrelated places).

I named it like this because that's how it's named in the redis source code skip_checksum_validation. Also in the redis it only can be se as a build option and not in redis.conf.

it is fine to diverge from other resp caches in such respects. having a conf option is clearly more powerful, and we can name it in a way that makes sense for garnet.

PaulusParssinen · 2025-01-17T19:53:05Z

libs/common/RespLengthEncodingUtils.cs

+    public static (long Length, byte PayloadStart) DecodeLength(ref ReadOnlySpan<byte> buff)
+    {
+        // remove the value type byte
+        var encoded = buff.Slice(1);
+
+        if (encoded.Length == 0)
+        {
+            throw new ArgumentException("Encoded length cannot be empty.", nameof(encoded));
+        }
+
+        var firstByte = encoded[0];
+        return (firstByte >> 6) switch
+        {
+            // 6-bit encoding
+            0 => (firstByte & 0x3F, 1),
+            // 14-bit encoding
+            1 when encoded.Length < 2 => throw new ArgumentException("Not enough bytes for 14-bit encoding."),
+            1 => (((firstByte & 0x3F) << 8) | encoded[1], 2),
+            // 32-bit encoding
+            2 when encoded.Length < 5 => throw new ArgumentException("Not enough bytes for 32-bit encoding."),
+            2 => ((long)((encoded[1] << 24) | (encoded[2] << 16) | (encoded[3] << 8) | encoded[4]), 5),
+            _ => throw new ArgumentException("Invalid encoding type.", nameof(encoded))
+        };
+    }


I'd like to suggest change of approach to this length decoding. Exception handling is expensive, to avoid it you can use the Try-pattern seen a lot in the .NET world. This way caller can decide how to handle failure, i.e. throw expensive exception or to maybe just write RESP error without EH catching logic?

The method should also only concern itself with the length decoding and leave the slicing of value type byte in the beginning to the caller who hands the input buffer to this method.

My alternate approach:

public static bool TryReadLength(ReadOnlySpan<byte> input, out int length, out int bytesRead) { length = 0; bytesRead = 0; if (input.Length < 1) return false; var firstByte = input[0]; switch (firstByte >> 6) { case 0: bytesRead = 1; length = firstByte & 0x3F; return true; case 1 when input.Length > 1: bytesRead = 2; length = ((firstByte & 0x3F) << 8) | input[1]; return true; case 2: bytesRead = 5; return BinaryPrimitives.TryReadInt32BigEndian(input.Slice(1), out length); default: return false; } }

With this approach, the JITted assembly will be much tighter: https://www.diffchecker.com/UMTqkaAE/

PaulusParssinen · 2025-01-17T20:00:20Z

libs/common/RespLengthEncodingUtils.cs

+    /// <param name="length"></param>
+    /// <returns></returns>
+    /// <exception cref="ArgumentOutOfRangeException"></exception>
+    public static byte[] EncodeLength(long length)


I agree bit @kevin-montrose here. A common approach is to have small static method to first calculate the required buffer size (it can be simple switch expression) for the caller (i.e. in our case NetworkDUMP method) to know how much buffer it needs (to slice from existing buffer, stackalloc, rent etc., doesn't matter).

Then the actual encoding method takes a Span<byte> output argument which ownership belongs to caller. Now we could do the encoding on this buffer without allocations. You can see this pattern all over .NET with TryWrite and other encoding/serialization methods.

PaulusParssinen · 2025-01-17T23:35:48Z

libs/server/Resp/KeyAdminCommands.cs

+            var buffer = totalLength <= 128
+                ? stackalloc byte[totalLength]
+                : ArrayPool<byte>.Shared.Rent(totalLength);


Excellent! This rent still needs to be returned to the ArrayPool<byte> after we're done. I'd also recommend bumping the threshold to 256 bytes for it to be worth it.

This is the common pattern:

byte[]? rentedBuffer = null; var buffer = totalLength <= 256 ? stackalloc byte[256] // constant sized stackallocs result better codegen (We should bunch of stack left here). : (rentedBuffer = ArrayPool<byte>.Shared.Rent(totalLength));

then after we're done:

if (rentedBuffer is not null) ArrayPool<byte>.Shared.Return(rentedBuffer);

PaulusParssinen · 2025-01-17T23:45:36Z

libs/common/RespLengthEncodingUtils.cs

+                bytesRead = 1;
+                length = firstByte & 0x3F;
+                return true;
+            case 1 when input.Length > 2:


I had small mistake in my snippet, sorry! 😅 (Off by ones, am I right?)

Suggested change

case 1 when input.Length > 2:

case 1 when input.Length > 1:

Implement basic version of DUMP and RESTORE redis commands

2954d6e

s3w3nofficial force-pushed the s3w3nofficial/add-dump-and-restore-command branch from f9c9838 to 2954d6e Compare January 3, 2025 01:24

Merge branch 'main' into s3w3nofficial/add-dump-and-restore-command

0e7f821

badrishc reviewed Jan 6, 2025

View reviewed changes

libs/common/RedisLengthEncodingUtils.cs Outdated Show resolved Hide resolved

TalZaccai requested a review from vazois January 7, 2025 19:28

vazois requested changes Jan 9, 2025

View reviewed changes

libs/common/Crc64.cs Show resolved Hide resolved

libs/server/Resp/Parser/RespCommand.cs Outdated Show resolved Hide resolved

libs/server/Resp/Parser/RespCommand.cs Outdated Show resolved Hide resolved

s3w3nofficial and others added 3 commits January 12, 2025 21:52

refactor and add dump, restore to cluster slot verification test

7406d74

Merge branch 'main' into s3w3nofficial/add-dump-and-restore-command

8a9ee0a

Update comments to use 'RESP' encoding terminology

6133607

TalZaccai linked an issue Jan 14, 2025 that may be closed by this pull request

DUMP Command Not Supported in Garnet #689

Open

s3w3nofficial added 2 commits January 15, 2025 23:09

fix formating

c14f0b3

Merge branch 'main' into s3w3nofficial/add-dump-and-restore-command

0d18b6a

vazois approved these changes Jan 15, 2025

View reviewed changes

libs/common/Crc64.cs Show resolved Hide resolved

fix tests

2f9b457

s3w3nofficial mentioned this pull request Jan 16, 2025

Can I quickly import rdb data? #412

Open

s3w3nofficial added 2 commits January 16, 2025 03:22

add acl and tests and update default config to include the skip checksum

2fa9b08

validation flag

rm accidentally commited dump.rdb file

181d780

vazois self-requested a review January 16, 2025 17:50

vazois requested changes Jan 16, 2025

View reviewed changes

libs/server/Resp/Parser/RespCommand.cs Outdated Show resolved Hide resolved

libs/server/Resp/Parser/RespCommand.cs Outdated Show resolved Hide resolved

vazois requested changes Jan 16, 2025

View reviewed changes

libs/server/Resp/RespServerSession.cs Outdated Show resolved Hide resolved

TalZaccai reviewed Jan 16, 2025

View reviewed changes

Remove trailing whitespace in RespCommandTests.cs

2a789d8

s3w3nofficial added 2 commits January 16, 2025 21:54

fix comments

f15a662

run CommandInfoUpdater and replace docs / info files

e281e34

Merge branch 'main' into s3w3nofficial/add-dump-and-restore-command

d41421d

badrishc and others added 2 commits January 16, 2025 13:25

Remove trailing whitespace in Options.cs

737e7a9

fix RestoreACLsAsync test

eed596f

kevin-montrose requested changes Jan 16, 2025

View reviewed changes

fix comments

85d0136

badrishc reviewed Jan 17, 2025

View reviewed changes

PaulusParssinen reviewed Jan 17, 2025

View reviewed changes

optimize RespLengthEncodingUtils

2f2a9a3

PaulusParssinen reviewed Jan 17, 2025

View reviewed changes

implement suggestions

024362b

@@ @@ -2652,6 +2652,50 @@ @@
                     }
                   ]
                 },
+                {


		var status = storageApi.GET(key, out var value);

		switch (status)

Adding basic version of DUMP and RESTORE commands #899

Are you sure you want to change the base?

Adding basic version of DUMP and RESTORE commands #899

Conversation

s3w3nofficial commented Jan 2, 2025

Implemented

Not Implemented

s3w3nofficial commented Jan 2, 2025

badrishc commented Jan 6, 2025

vazois commented Jan 14, 2025

s3w3nofficial commented Jan 15, 2025

vazois left a comment

Choose a reason for hiding this comment

s3w3nofficial commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badrishc commented Jan 16, 2025

kevin-montrose commented Jan 16, 2025

kevin-montrose left a 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

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

PaulusParssinen Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

s3w3nofficial commented Jan 16, 2025

badrishc Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badrishc Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

PaulusParssinen Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

PaulusParssinen Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

PaulusParssinen Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulusParssinen Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulusParssinen Jan 17, 2025 •

edited

Loading

badrishc Jan 17, 2025 •

edited

Loading

badrishc Jan 17, 2025 •

edited

Loading

PaulusParssinen Jan 17, 2025 •

edited

Loading

PaulusParssinen Jan 17, 2025 •

edited

Loading

PaulusParssinen Jan 17, 2025 •

edited

Loading

PaulusParssinen Jan 17, 2025 •

edited

Loading