Skip to content

Module api support for RESP3#8521

Merged
oranagra merged 15 commits intoredis:unstablefrom
ashtul:ariel-module_api_resp3
Aug 3, 2021
Merged

Module api support for RESP3#8521
oranagra merged 15 commits intoredis:unstablefrom
ashtul:ariel-module_api_resp3

Conversation

@ashtul
Copy link
Contributor

@ashtul ashtul commented Feb 22, 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

@ashtul ashtul marked this pull request as ready for review February 23, 2021 12:07
@guybe7 guybe7 mentioned this pull request Mar 22, 2021
@oranagra
Copy link
Member

oranagra commented Jul 7, 2021

@ashtul do you wanna pick this one up again, or shall i take over?
IIRC there are few minor changes to make, and also drop "push" feature for now and leave it for some future PR.
@MeirShpilraien please take a quick look.

@ashtul
Copy link
Contributor Author

ashtul commented Jul 8, 2021

@oranagra I will come back to it next week

Copy link

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

👍
One important comment about attribute replies

Co-authored-by: Oran Agra <[email protected]>
Copy link
Contributor Author

@ashtul ashtul left a comment

Choose a reason for hiding this comment

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

comments after a review with @oranagra

@oranagra
Copy link
Member

@MeirShpilraien @ashtul @zuiderkwast please take a look when you get a chance.

@oranagra
Copy link
Member

@redis/core-team please approve (description at the top)

@oranagra oranagra added approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Jul 22, 2021
oranagra
oranagra previously approved these changes Jul 25, 2021
yossigo
yossigo previously approved these changes Jul 25, 2021
@oranagra oranagra dismissed stale reviews from yossigo and themself via 0558b5f July 25, 2021 10:44
Co-authored-by: Yossi Gottlieb <[email protected]>
This was referenced Jul 25, 2021
@oranagra oranagra merged commit bdbf5ee into redis:unstable Aug 3, 2021
@ashtul ashtul deleted the ariel-module_api_resp3 branch August 4, 2021 09:24
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

8 participants