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

Automation - Lock reads and writes from and to the ConfigDigest #14313

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

ferglor
Copy link
Collaborator

@ferglor ferglor commented Sep 3, 2024

This data race was flagged up in #topic-data-races

The change here is adding a RWMutex to synchronize reads and writes from/to the ConfigDigest

@ferglor ferglor requested review from a team as code owners September 3, 2024 10:05
@ferglor ferglor requested review from reductionista and removed request for a team September 3, 2024 10:13
@ferglor ferglor force-pushed the fix/automation-telemetry-data-race branch from 5f0c659 to 8297979 Compare September 3, 2024 10:14
infiloop2
infiloop2 previously approved these changes Sep 3, 2024
…metry21/custom_telemetry.go

Co-authored-by: Jordan Krage <jmank88@gmail.com>
@ferglor ferglor requested review from jmank88 and infiloop2 September 3, 2024 15:08
Comment on lines +79 to 81
if configChanged {
e.sendNodeVersionMsg()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why change this behaviour of only sending this message on config change and not on regular intervals?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the previous version of the code, we only sent the message when the config had changed:

We need to lock the mutex before checking if the config has changed (the if statement); but because the sendNodeVersionMsg itself locks the Read lock on the mutex (because it gets called from other places, too), we can't execute the sendNodeVersionMsg function while the lock is held. And so, we're having to introduce this additional boolean that gets set to true when the config changes (previously, this is when we would have called sendNodeVersionMsg), and then reference that boolean outside the lock to determine if we should call sendNodeVersionMsg.

Copy link
Contributor

@infiloop2 infiloop2 Sep 4, 2024

Choose a reason for hiding this comment

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

ah sorry had missed that e.sendNodeVersionMsg() was within the if statement before. There's a separate ticker which sends this message on regular intervals. Thanks for clarifying!

Copy link
Contributor

@infiloop2 infiloop2 left a comment

Choose a reason for hiding this comment

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

have a question about change of behaviour

@cl-sonarqube-production
Copy link

@ferglor ferglor added this pull request to the merge queue Sep 4, 2024
Merged via the queue into develop with commit b71e692 Sep 4, 2024
135 checks passed
@ferglor ferglor deleted the fix/automation-telemetry-data-race branch September 4, 2024 15:30
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.

3 participants