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.

Sign up for GitHub

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

Jump to bottom

Drop stuck lookups #5824

Merged
merged 1 commit into from
May 23, 2024
Merged

Drop stuck lookups #5824

merged 1 commit into from
May 23, 2024

Conversation

dapplion
Copy link
Collaborator

Issue Addressed

PR

Makes us aware of the extent of the problem of sync lookups randomly getting stuck.

While useful I believe we should:

  • Take it one step further and actively drop lookups that get stuck
  • Increase the log level of "lookup stuck alarm"

While

  • Fix underlying bugs that cause lookups to get stuck, both find by us and reported by users

The trade-off is quite good:

  • A healthy lookup needing to exist for > 16 minutes is highly unlikely. Even if it's the case, range sync can rescue the node eventually
  • So far, bugs that cause lookups to get stuck are sporadic, usually dependent of a specific sequence of events that may not repeat in a while.

Therefore dropping stuck lookups should be a net positive

Proposed Changes

  • If a lookup lasts more than LOOKUP_MAX_DURATION_SECS this function will find its oldest ancestor and then drop it and all its children.
  • Increase the "lookup stuck" log from debug to warn, as now it will log only once per lookup id

@dapplion dapplion mentioned this pull request May 22, 2024
Open
@AgeManning
Copy link
Member

This seems reasonable to me.

@michaelsproul michaelsproul added ready-for-review The code is ready for review v5.2.0 Q2 2024 labels May 23, 2024
michaelsproul added a commit that referenced this pull request May 23, 2024
@michaelsproul
Drop stuck lookups (#5824)
acc5eef 
Squashed commit of the following:

commit f15131f
Author: dapplion <[email protected]>
Date:   Wed May 22 16:01:22 2024 +0200

    Drop stuck lookups
@michaelsproul michaelsproul mentioned this pull request May 23, 2024
Merged
jimmygchen
jimmygchen approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels May 23, 2024
@jimmygchen
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented May 23, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 17d9086

mergify bot added a commit that referenced this pull request May 23, 2024
@mergify
Merge of #5824
8cec4e1
mergify bot added a commit that referenced this pull request May 23, 2024
@mergify
Merge of #5824
0b0dbf8
This was referenced May 23, 2024
Closed
Merged
@mergify mergify bot merged commit 17d9086 into sigp:unstable May 23, 2024
28 checks passed
@dapplion dapplion deleted the drop-stuck-lookups branch May 23, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgeManning AgeManning AgeManning approved these changes

@jimmygchen jimmygchen jimmygchen approved these changes

@realbigsean realbigsean Awaiting requested review from realbigsean

Assignees
No one assigned
Labels
ready-for-merge This PR is ready to merge. v5.2.0 Q2 2024
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dapplion @AgeManning @jimmygchen @michaelsproul