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

Use RWMutex in demultiplexer to prevent check delays #26148

Merged
merged 1 commit into from
May 30, 2024

Conversation

gh123man
Copy link
Member

@gh123man gh123man commented May 30, 2024

What does this PR do?

Followup to this PR: #25858

Very long running flushes can cause the lock in demultiplexer_agent to be held while checks are starting. When checks start they call GetSender to acquire their sender instance which shares the same lock as flush. As a result, a very long running flush will block all checks from starting until it completes.

In order to avoid check execution delays, we can use an RWMutex here since the protected state is only mutated during Stop. So RLock will never block during normal agent operations allowing checks to start asynchronously of the flush operation.

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

This was deployed in a large staging cluster so QA is done.

@gh123man gh123man added changelog/no-changelog team/agent-metrics-logs qa/done QA done before merge and regressions are covered by tests labels May 30, 2024
@gh123man gh123man added this to the 7.55.0 milestone May 30, 2024
@gh123man gh123man changed the title Use RWMutex in demultiplexer agent Use RWMutex in demultiplexer to prevent check delays May 30, 2024
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.70%. Comparing base (c7efe43) to head (7403eac).
Report is 42 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main   #26148       +/-   ##
===========================================
+ Coverage   45.06%   57.70%   +12.63%     
===========================================
  Files        2338      778     -1560     
  Lines      269660    64798   -204862     
===========================================
- Hits       121532    37393    -84139     
+ Misses     138511    25505   -113006     
+ Partials     9617     1900     -7717     
Flag Coverage Δ
amzn_aarch64 58.79% <100.00%> (+12.90%) ⬆️
centos_x86_64 58.69% <100.00%> (+12.89%) ⬆️
ubuntu_aarch64 58.80% <100.00%> (+12.91%) ⬆️
ubuntu_x86_64 58.78% <100.00%> (+12.90%) ⬆️
windows_amd64 57.36% <100.00%> (+5.97%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented May 30, 2024

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=35487830 --os-family=ubuntu

@gh123man gh123man marked this pull request as ready for review May 30, 2024 17:44
@gh123man gh123man requested a review from a team as a code owner May 30, 2024 17:44
@gh123man
Copy link
Member Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented May 30, 2024

🚂 MergeQueue

Pull request added to the queue.

There are 2 builds ahead! (estimated merge in less than 1h)

Use /merge -c to cancel this operation!

@dd-mergequeue dd-mergequeue bot merged commit 696595c into main May 30, 2024
237 checks passed
@dd-mergequeue dd-mergequeue bot deleted the brian/demultiplexer-agent-sender-rwlock-main branch May 30, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog qa/done QA done before merge and regressions are covered by tests team/agent-metrics-logs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants