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

xtrim parameters regression? #283

Closed
kwisath opened this issue Jun 26, 2023 · 4 comments
Closed

xtrim parameters regression? #283

kwisath opened this issue Jun 26, 2023 · 4 comments
Assignees
Labels
Milestone

Comments

@kwisath
Copy link

kwisath commented Jun 26, 2023

On the commands.edn file of commit 5f9f6b2 XTRIM is defined as follows

"XTRIM" {:fn-name "xtrim", 
:fn-docstring "Trims the stream to (approximately if '~' is passed) a certain size.\n\nXTRIM key trim\n\nAvailable since: 5.0.0.\n\nTime complexity: O(N), with N being the number of evicted entries. Constant times are very small however, since entries are organized in macro nodes containing multiple entries that can be released with a single deallocation.", 
:fn-params-fixed [key trim], 
:fn-params-more nil, 
:req-args-fixed ["XTRIM" key trim], 
:cluster-key-idx 1}

while the traditional way in which it was defined was

"XTRIM" {:fn-name "xtrim", 
:fn-docstring "Trims the stream to (approximately if '~' is passed) a certain size.\n\nXTRIM key MAXLEN [~] count\n\nAvailable since: 5.0.0.\n\nTime complexity: O(N), with N being the number of evicted entries. Constant times are very small however, since entries are organized in macro nodes containing multiple entries that can be released with a single deallocation.", 
:fn-params-fixed [key strategy], 
:fn-params-more [key strategy & args], 
:req-args-fixed ["XTRIM" key strategy], 
:cluster-key-idx 1}

Note that the new version has :fn-params-more nil which prevents xtrim from being called like (car/xtrim stream :MINID "~" last-ts). Per https://redis.io/commands/xtrim/ this type of invocation should be possible (and was totally possible and working as a charm in previous versions of the library due to the & args in :fn-params-more).

@ptaoussanis
Copy link
Member

@kwisath Hi there, thanks for the report!

That's strange. Carmine's commands.edn file is produced directly from the official command spec.

If there's an incorrect entry in commands.edn, then it's either a regression in the official spec - or there's a bug in Carmine's command spec parser that appears to be triggering in only this case.

I'm planning to cut a major new Carmine release by the end of June, will take a closer look at this as part of that batched work.

As a temporary workaround in the meantime, you can always use Carmine's redis-call API to send arbitrary commands to Redis.

@ptaoussanis ptaoussanis self-assigned this Jun 26, 2023
@ptaoussanis ptaoussanis added this to the v3.3 milestone Jun 26, 2023
@kwisath
Copy link
Author

kwisath commented Jun 26, 2023

Thanks @ptaoussanis !
I had a look at the official command spec you linked and the XTRIM appears with a format that I think may be confusing the spec parser (from a semantic point of view the command in the JSON file does not seem to be wrong)

 "arguments": [
            {
                "name": "key",
                "type": "key",
                "display_text": "key",
                "key_spec_index": 0
            },
            {
                "name": "trim",
                "type": "block",
                "arguments": [
                    {
                        "name": "strategy",
                        "type": "oneof",
                        "arguments": [
                            {
                                "name": "maxlen",
                                "type": "pure-token",
                                "display_text": "maxlen",
                                "token": "MAXLEN"
                            },
                            {
                                "name": "minid",
                                "type": "pure-token",
                                "display_text": "minid",
                                "token": "MINID",
                                "since": "6.2.0"
                            }
                        ]
                    },
                    {
                        "name": "operator",
                        "type": "oneof",
                        "optional": true,
                        "arguments": [
                            {
                                "name": "equal",
                                "type": "pure-token",
                                "display_text": "equal",
                                "token": "="
                            },
                            {
                                "name": "approximately",
                                "type": "pure-token",
                                "display_text": "approximately",
                                "token": "~"
                            }
                        ]
                    },
                    {
                        "name": "threshold",
                        "type": "string",
                        "display_text": "threshold"
                    },
                    {
                        "name": "count",
                        "type": "integer",
                        "display_text": "count",
                        "token": "LIMIT",
                        "since": "6.2.0",
                        "optional": true
                    }
                ]
            }

The trim argument in fact has nested arguments inside of it, and maybe the command spec parser is not prepared to deal with that.

@ptaoussanis
Copy link
Member

The trim argument in fact has nested arguments inside of it, and maybe the command spec parser is not prepared to deal with that.

👍 PR welcome if you feel like taking a stab at updating the parser, otherwise I'll try take a look myself as part of the work for the upcoming v3.3.

Thanks again for pinging about this!

Cheers :-)

@ptaoussanis ptaoussanis added bug and removed bug? labels Jul 17, 2023
@ptaoussanis
Copy link
Member

Closing, this will be addressed in forthcoming release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants