-
Notifications
You must be signed in to change notification settings - Fork 111
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
[DATA] Don't acquire lock for whole duration of syncDataCaptureFiles. #1481
[DATA] Don't acquire lock for whole duration of syncDataCaptureFiles. #1481
Conversation
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.
[q] Should we also apply the same lock/unlock logic to buildAdditionalSyncPaths?
@tahiyasalam very good catch, we absolutely should. Especially since that's also doing some relatively long running work (some disk io). Updated following the same pattern |
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.
Neat fix, LGTM!
|
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.
Thanks for the fix! LGTM and know this is merged - just one question.
We don't need to acquire this lock for the duration of the function, only when accessing the shared variable svc.collectors, which we can copy.
Also replaces the behavior when a data capture file can't be created to return an err instead of log an error. We can't capture or sync data if that doesn't work, so I think it should fail quickly/completely vs just logging.