Module api support for RESP3#8521
Merged
oranagra merged 15 commits intoredis:unstablefrom Aug 3, 2021
Merged
Conversation
zuiderkwast
reviewed
Mar 1, 2021
oranagra
reviewed
Mar 21, 2021
Closed
Member
|
@ashtul do you wanna pick this one up again, or shall i take over? |
Contributor
Author
|
@oranagra I will come back to it next week |
MeirShpilraien
left a comment
There was a problem hiding this comment.
👍
One important comment about attribute replies
2 tasks
ashtul
commented
Jul 13, 2021
oranagra
reviewed
Jul 13, 2021
Co-authored-by: Oran Agra <[email protected]>
Member
|
@MeirShpilraien @ashtul @zuiderkwast please take a look when you get a chance. |
Member
|
@redis/core-team please approve (description at the top) |
oranagra
previously approved these changes
Jul 25, 2021
yossigo
previously approved these changes
Jul 25, 2021
Co-authored-by: Yossi Gottlieb <[email protected]>
oranagra
approved these changes
Jul 25, 2021
This was referenced Jul 25, 2021
madolson
approved these changes
Aug 3, 2021
JackieXie168
pushed a commit
to JackieXie168/redis
that referenced
this pull request
Sep 8, 2021
Add new Module APS for RESP3 responses: - RM_ReplyWithMap - RM_ReplyWithSet - RM_ReplyWithAttribute - RM_ReplySetMapLength - RM_ReplySetSetLength - RM_ReplySetAttributeLength - RM_ReplyWithBool Deprecate REDISMODULE_POSTPONED_ARRAY_LEN in favor of a generic REDISMODULE_POSTPONED_LEN Improve documentation Add tests Co-authored-by: Guy Benoish <[email protected]> Co-authored-by: Oran Agra <[email protected]>
oranagra
added a commit
that referenced
this pull request
Oct 25, 2021
Let modules use additional type of RESP3 response (unused by redis so far) Also fix tests that where introduced in #8521 but didn't actually run. Co-authored-by: Oran Agra <[email protected]>
oranagra
added a commit
that referenced
this pull request
Nov 1, 2021
The module test in reply.tcl was introduced by #8521 but didn't run until recently (see #9639) and then it started failing with valgrind. This is because valgrind uses 64 bit long double (unlike most other platforms that have at least 80 bits) But besides valgrind, the tests where also incompatible with ARM32, which also uses 64 bit long doubles. We now use appropriate value to avoid issues with either valgrind or ARM32 In all the double tests, i use 3.141, which is safe since since addReplyDouble uses `%.17Lg` which is able to represent this value without adding any digits due to precision loss. In the long double, since we use `%.17Lf` in ld2string, it preserves 17 significant digits, rather than 17 digit after the decimal point (like in `%.17Lg`). So to make these similar, i use value lower than 1 (no digits left of the period) Lastly, we have the same issue with TCL (no long doubles) so we read raw protocol in that test. Note that the only error before this fix (in both valgrind and ARM32 is this: ``` *** [err]: RM_ReplyWithLongDouble: a float reply in tests/unit/moduleapi/reply.tcl Expected '3.141' to be equal to '3.14100000000000001' (context: type eval line 2 cmd {assert_equal 3.141 [r rw.longdouble 3.141]} proc ::test) ``` so the changes to debug.c and scripting.tcl aren't really needed, but i consider them a cleanup (i.e. scripting.c validated a different constant than the one that's sent to it from debug.c). Another unrelated change is to add the RESP version to the repeated tests in reply.tcl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add new Module APS for RESP3 responses:
Deprecate REDISMODULE_POSTPONED_ARRAY_LEN in favor of a generic REDISMODULE_POSTPONED_LEN
Improve documentation
Add tests