-
Notifications
You must be signed in to change notification settings - Fork 410
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(flow): add ignoreDependencyOnFailure option #2426
Conversation
0b73bfd
to
35582a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment.
@@ -50,11 +51,21 @@ local function removeJob( prefix, jobId, parentKey, removeChildren) | |||
removeJob( childJobPrefix, childJobId, jobKey, removeChildren ) | |||
end | |||
end | |||
|
|||
local failed = rcall("HGETALL", jobKey .. ":failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call could potentially require a lot of memory and resources if the number of failed jobs is very large. A solution would be to use "https://redis.io/commands/hlen/" first, and if the len is larger than a specific size, maybe 100 or so, start a iteration process. Obviously this is going to be complicated in general as the iteration would need to be done by calling "removeJob" iteratively... Maybe it is not critical to get this done in this PR, but we should mark it in an issue as a known issue/future enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I referenced one issue to remember this improvement
# [5.2.0](v5.1.12...v5.2.0) (2024-02-17) ### Features * **flow:** add ignoreDependencyOnFailure option ([#2426](#2426)) ([c7559f4](c7559f4))
ref #2092 #800