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

fix(admin): fix snwatcher handle heartbeat timeout #1070

Merged
merged 2 commits into from
Apr 7, 2025

Conversation

hungryjang
Copy link
Member

What this PR does

Which issue(s) this PR resolves

This change fixes an issue where not all expected logstreams could be sealed within HandleHeartbeatTimeout due to storagenode delaying the response. Also fixes an issue where unexpected logstreams were being sealed by heartbeat false positives. In particular, it increases the priority of seal requests for metarepos so that commit results in metarepos do not pile up even if sealing is delayed.

Resolves #1068 #1069

Anything else

Include any links or documentation that might be helpful for reviewers.

@hungryjang hungryjang requested a review from ijsong as a code owner April 4, 2025 01:02
@hungryjang hungryjang changed the base branch from main to test_snwatcher_sn_timeout April 4, 2025 01:08
Base automatically changed from test_snwatcher_sn_timeout to main April 4, 2025 01:09
@hungryjang hungryjang force-pushed the fix-snwatcher-handle-heartbeat-timeout branch from 9e54fd2 to 8c3a241 Compare April 4, 2025 01:10
Copy link

codecov bot commented Apr 4, 2025

Codecov Report

Attention: Patch coverage is 86.23188% with 19 lines in your changes missing coverage. Please review.

Project coverage is 79.35%. Comparing base (98f379a) to head (66c594e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/admin/admin.go 66.66% 8 Missing and 2 partials ⚠️
internal/admin/mrmanager/manager.go 82.69% 8 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1070      +/-   ##
==========================================
- Coverage   79.71%   79.35%   -0.37%     
==========================================
  Files         178      178              
  Lines       21564    21637      +73     
==========================================
- Hits        17190    17170      -20     
- Misses       3591     3674      +83     
- Partials      783      793      +10     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This change fixes an issue where not all expected logstreams could be sealed
within HandleHeartbeatTimeout due to storagenode delaying the response.
Also fixes an issue where unexpected logstreams were being sealed by
heartbeat false positives. In particular, it increases the priority of seal
requests for metarepos so that commit results in metarepos do not pile up
even if sealing is delayed.
@hungryjang hungryjang force-pushed the fix-snwatcher-handle-heartbeat-timeout branch from 8c3a241 to ba419aa Compare April 4, 2025 07:24
Copy link
Member

@ijsong ijsong left a comment

Choose a reason for hiding this comment

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

Nice work!

@hungryjang hungryjang merged commit 36ebaa8 into main Apr 7, 2025
16 of 18 checks passed
@hungryjang hungryjang deleted the fix-snwatcher-handle-heartbeat-timeout branch April 7, 2025 00:52
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.

admin: There are cases where not all expected ls are sealed even if a storagenode heartbeat timeout occurs.
2 participants