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

Archived tasks that are trimmed from the set are deleted #743

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Harrison-Miller
Copy link

Solves the issue outlined in: #730

You can confirm the test case/fix is working by deleting lines: 852/860 in rdb.go of the fix branch to observe the old failure.

@Harrison-Miller Harrison-Miller changed the title Archived tasks that are trimmed from the set are deteleted Archived tasks that are trimmed from the set are deleted Sep 19, 2023
@kamikazechaser
Copy link
Collaborator

Depends on #441

internal/rdb/rdb.go Outdated Show resolved Hide resolved
local extra = redis.call("ZRANGE", KEYS[4], 0, -ARGV[5])
if #extra > 0 then
for _, id in ipairs(extra) do
redis.call("DEL", KEYS[9] .. id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, we need to raise the maxArchiveSize or make it settable before calling delete here. In a scenario where tasks are failing and reaching max retries fast, it will just keep clearing them from the queue before allowing for manual intervention.

I see it is being tracked in #534 already :)

Copy link
Collaborator

@kamikazechaser kamikazechaser left a comment

Choose a reason for hiding this comment

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

Thanks, changes look good to me. I have left a suggestion.

@ray2011
Copy link

ray2011 commented May 9, 2024

When will this pull request be merged?

@kamikazechaser
Copy link
Collaborator

@ray2011 It (softly) depends on the #534.

@Harrison-Miller
Copy link
Author

I will try and work on the suggestion in the next few weeks.

@Harrison-Miller
Copy link
Author

@kamikazechaser nvm it took 3 seconds to change, confirmed the tests I added still pass. Anything else you need changed?

@kamikazechaser
Copy link
Collaborator

Anything else you need changed?

No, Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants