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

feat: Add get_progress and set_progress to redis result backend #67

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

sminnee
Copy link
Contributor

@sminnee sminnee commented Jul 24, 2024

Uses as standard suffix on the redis key (hardcoded as "__progress") to store progress results.

@sminnee
Copy link
Contributor Author

sminnee commented Jul 25, 2024

A few questions about this:

  • Is a key with suffix __progress a good default? Should the suffix be configurable?
  • When should the data expire? Using redis.getdel() as with get_results is probably not appropriate as it's much more likely to be fetched repeatedly, but maybe it should have a shorter default expiry time? (2h?)
  • Should it be possible to configure progress expiry separately from results expiry?

I also note that there's a fair bit of duplication in RedisAsyncClusterResultBackend, RedisAsyncResultBackend, and RedisAsyncSentinelResultBackend. They could possibly be refactored to have a common base class with most of the implementation, and the subclass just defining a method that provides the redis connection object as a resource (i.e. with enter() and exit() even if it's not strictly necessary). That refactor is beyond the scope of this PR, but I'm happy to do it.

CC @Sobes76rus as you did the original set_/get_progress implementation

@Sobes76rus
Copy link

Sobes76rus commented Jul 25, 2024

Hello, I think the __progress suffix is good enough and doesn't need configuration
Expiration should be customisable, and it would be even better if it is customisable for each task for results and for progress based on labels

@sminnee
Copy link
Contributor Author

sminnee commented Jul 25, 2024

OK so should I add 2 more arguments to the result backend(s) constructor?

  • progress_ex_time: Optional[int] = 600
  • progress_px_time: Optional[int] = None

I'm happy to add that to this PR, if @s3rius gives a 👍.

The bit I'm least confident about is the 600 default - it's a little arbitrary, but I think some default is better than no expiry. My rationale is that if there have been no progress updates in the last 10 minutes, something has probably gone wrong.

@sminnee
Copy link
Contributor Author

sminnee commented Jul 27, 2024

FYI I believe I've fixed the build now, sorry about missing that in the first PR!

@sminnee
Copy link
Contributor Author

sminnee commented Aug 4, 2024

@s3rius it would be great to get your input on this, if you have any? :)

@sminnee
Copy link
Contributor Author

sminnee commented Aug 16, 2024

Hey, just giving this a nudge. I'm running my project on a fork so no great hurry, although it would be good to know if you're on board with the direction I took?

@s3rius
Copy link
Member

s3rius commented Aug 18, 2024

It looks good to me. Let's check tests and release it if it's fine.

@s3rius s3rius changed the base branch from develop to main August 18, 2024 09:16
@s3rius
Copy link
Member

s3rius commented Aug 18, 2024

Can you please rebase onto the main branch? I've fixed the workflows.

Uses as standard suffix on the redis key (hardcoded as "__progress") to store progress results
@sminnee
Copy link
Contributor Author

sminnee commented Aug 28, 2024

done

@pdjohntony
Copy link

Any ETA on this?

@s3rius s3rius merged commit 254fae4 into taskiq-python:main Sep 26, 2024
@sminnee sminnee deleted the feature/progress branch September 29, 2024 20:34
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.

4 participants