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.

Sign up for GitHub

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

Jump to bottom

Add support for TIMEOUT in FT.Aggregate and FT.Search #2486

Closed
fast-facts opened this issue Apr 28, 2023 · 5 comments · Fixed by #2488
Closed

Add support for TIMEOUT in FT.Aggregate and FT.Search #2486

fast-facts opened this issue Apr 28, 2023 · 5 comments · Fixed by #2488
Assignees
@Nanak360
Labels
Good First Issue help-wanted

Comments

@fast-facts
Copy link
Contributor

Motivation

Provides support for the TIMEOUT argument in FT.Aggregate and FT.Search

Basic Code Example

await client.ft.search('idx', 'query', { RETURN: [], TIMEOUT: 5000 })
await client.ft.aggregate('idx', 'query', { STEPS: [...], TIMEOUT: 5000 })
@Nanak360
Copy link
Contributor

I would love to take this up

@leibale
Copy link
Collaborator

leibale commented Apr 28, 2023

@Nanak360 go for it.. :)
Thanks for contributing!!

@leibale
Copy link
Collaborator

leibale commented Apr 28, 2023

https://github.com/redis/node-redis/blob/master/packages/search/lib/commands/AGGREGATE.ts
https://github.com/redis/node-redis/blob/master/packages/search/lib/commands/SEARCH.ts
@Nanak360 make sure to add tests as well :)

Nanak360 added a commit to Nanak360/node-redis that referenced this issue Apr 28, 2023
@Nanak360
redis#2486: add timeout as optional param in FT.Search
70ed49e
@Nanak360
Copy link
Contributor

Hey @leibale, I've created a draft PR for this issue.
I have some doubts and would really appreciate your feedback or someone else's review of the changes.

I have added TIMEOUT as an optional parameter, but I'm a bit confused about the next steps. Currently, I have only included TIMEOUT in the return arguments.

You can find the draft PR here, and any modifications or suggestions you have would be greatly appreciated.

@leibale leibale linked a pull request Apr 29, 2023 that will close this issue
Add support for TIMEOUT in FT.AGGREGATE and FT.SEARCH #2488
Merged
@leibale
Copy link
Collaborator

leibale commented Apr 29, 2023

@Nanak360 I left one comment, but overall looks good! Good job! :)

@leibale leibale mentioned this issue May 21, 2023
Merged
leibale pushed a commit that referenced this issue May 21, 2023
@Nanak360
Add support for TIMEOUT in FT.AGGREGATE and FT.SEARCH (#2488)
8d37c11 
* #2486: add timeout as optional param in FT.Search

* return timeout from aggregate

* add test case for TIMEOUT in aggregate

* add TIMEOUT option in search file

* add test cases for TIMEOUT option in search file

* uodate search/aggregates to add timeout when it is not undefuned

* update search to add timeout when it is not undefuned

* update test case for AGGREGATE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Nanak360 Nanak360

Labels
Good First Issue help-wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for TIMEOUT in FT.AGGREGATE and FT.SEARCH Nanak360/node-redis
3 participants
@leibale @Nanak360 @fast-facts