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

Adding ROLE command #964

Merged
merged 14 commits into from
Jan 29, 2025
Merged

Adding ROLE command #964

merged 14 commits into from
Jan 29, 2025

Conversation

prvyk
Copy link
Contributor

@prvyk prvyk commented Jan 27, 2025

An initial attempt to add the ROLE command + docs + tests. Likely this will need some revisions.

https://redis.io/docs/latest/commands/role/

  1. After Fix append after expiration #961 the new ACL test blows up (it passed previously). I suspect that introduced an unrelated regression affecting this test, since we just use normal serialization methods.
  2. I'm not sure the replication state check is correct at all. Note we check the new IsPrimary() since the recovering case should go to the replica branch unlike normal IsReplica().
  3. Didn't mention 'sentinel' possibility in docs, as Garnet does not support this mode.
  4. Far from sure this is the best code organization, it seemed cleaner than doing the logic in one method.
  5. Minor effect on Metrics code path (see first commit), but with same result.

@badrishc
Copy link
Contributor

I don't think the unit test error is related to #961. It might be related to #925. The issue is that GarnetClient cannot handle an array response where one of the array elements is itself another array. It is possible that prior to #925, we were swallowing this somehow?

RECV: [*1|$4|ROLE|]
SEND: [*3|$6|master|:0|*0|]
Exception thrown: 'Garnet.common.Parsing.RespParsingException' in Garnet.common.dll
Unexpected character '*'.

One solution would be to add basic support for an array field in RespReadResponseUtils.cs line 192, something like this:

                else if (*ptr == ':')
                {
                    if (!TryReadIntegerAsString(out result[i], ref ptr, end))
                        return false;
                }
                else if (*ptr == '*')
                {
                    if (!TryReadStringArrayWithLengthHeader(out var subArray, ref ptr, end))
                        return false;
                    result[i] = string.Join(", ", subArray);
                }
                else
                {
                    RespParsingException.ThrowUnexpectedToken(*ptr);
                }

@prvyk
Copy link
Contributor Author

prvyk commented Jan 27, 2025

Thanks. Added this to unblock review for now. Perhaps this should be fixed separately, if it is it would be easy to revert/force push/ to get that out from the PR.

@badrishc
Copy link
Contributor

Feel free to split this change out to its own PR. It is not perfect, but we don't really plan to make GarnetClient perfect for all cases anyway. it just needs to handle these nestings gracefully.

Copy link
Contributor

@vazois vazois left a comment

Choose a reason for hiding this comment

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

Great work! Some minor comments

libs/common/Metrics/ReplicaInfo.cs Outdated Show resolved Hide resolved
libs/common/Metrics/ReplicaInfo.cs Outdated Show resolved Hide resolved
test/Garnet.test.cluster/ClusterRoleTests.cs Outdated Show resolved Hide resolved
test/Garnet.test.cluster/ClusterRoleTests.cs Outdated Show resolved Hide resolved
test/Garnet.test.cluster/ClusterRoleTests.cs Outdated Show resolved Hide resolved
@prvyk
Copy link
Contributor Author

prvyk commented Jan 28, 2025

The new CLUSTER NODES test has an interesting Linux-only failure... CLUSTER NODES is unrelated to this PR, I think it should get its own?

@vazois
Copy link
Contributor

vazois commented Jan 28, 2025

The new CLUSTER NODES test has an interesting Linux-only failure... CLUSTER NODES is unrelated to this PR, I think it should get its own?

Yes, revert the change it is not related to this PR. If you would like to address the issue in a separate PR please go ahead.

Copy link
Contributor

@vazois vazois left a comment

Choose a reason for hiding this comment

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

I just noticed some more things. Please address.

libs/cluster/Server/Replication/PrimaryOps/AofTaskStore.cs Outdated Show resolved Hide resolved
libs/cluster/Server/ClusterProvider.cs Outdated Show resolved Hide resolved
@vazois vazois requested review from vazois January 29, 2025 22:18
@vazois vazois merged commit 7d44d74 into microsoft:main Jan 29, 2025
15 checks passed
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.

3 participants