-
Notifications
You must be signed in to change notification settings - Fork 597
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
ISSUE-1830: Adding changes for stopping the seceret and malware scans #1316
Conversation
6d4326c
to
b758101
Compare
@@ -14,22 +14,37 @@ import ( | |||
) | |||
|
|||
var ( | |||
MaxStops = 5 |
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 MaxStops
is a bit misleading because it only applies for 1 iteration: Let's imagine we cancel 30 scans, we will do: 5 cancel, next 30 seconds 5 more, next 30 seconds 5 more, without checking if the previous cancel were completed.
Now I think in practice we should be fine because we expect cancels to finish quickly and the request should not use extra resources to complete.. I would rather remove that MaxStops
entirely, just to simplify our logic and not mislead us in the future
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 believe we would need to put in a limit to how many we can process in a one go.
Maybe we can raise this to higher number (maybe 50)
type Entry struct { | ||
Posn int | ||
Data map[string]interface{} | ||
} |
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 would rather keep previous code here as this is not solving anything new from what I understand.
The allocation optimization looks fine:
statuses := make([]map[string]interface{}, len(statusBuff))
But the Posn
introduction is not solving anything new, is it?
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 is to maintain the ordering of the messages.
Right now we store conflated data in the map, and maps does not guarantee the ordering of the messages.
For instance, if we received the message in below order:
"IN_PROGRESS" -> "CANCELLED" status, we are not guaranteed to process the status in the same order.
Hence we might end up with a record in the neo4j which is in "IN_PROGRESS" status.
b758101
to
0e17447
Compare
0e17447
to
7880cf4
Compare
Fixes #
https://github.com/deepfence/enterprise-roadmap/issues/1830
Changes proposed in this pull request: