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

feat(share/p2p/discovery): Warning for discovery loop if haven't found wanted peers in 5 minutes #2573

Merged
merged 14 commits into from
Aug 25, 2023
Merged

Conversation

nodersteam
Copy link
Contributor

@nodersteam nodersteam commented Aug 15, 2023

refactor: Enhance peer discovery loop with connectivity warning

This commit refactors the discoveryLoop function in the Discovery struct
to include a connectivity warning timer. The timer is set to trigger every
5 minutes and checks if the number of discovered peers has not reached the
desired limit. If the limit is not met, a warning message is logged to
indicate potentially degraded connectivity.

Changes:

  • Added a warning timer to the discoveryLoop function.
  • The timer checks if the number of discovered peers is below the desired limit.
  • If the condition is met, a warning message is logged.

This enhancement improves the monitoring and reporting of connectivity issues
during the peer discovery process.

image

Closes #2474

@github-actions github-actions bot added the external Issues created by non node team members label Aug 15, 2023
@nodersteam nodersteam changed the title Enhance peer discovery loop with connectivity warning feat(share/p2p/discovery): Warning for discovery loop if haven't found wanted peers in 5 minutes Aug 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2023

Codecov Report

Merging #2573 (260d273) into main (1205437) will decrease coverage by 0.17%.
The diff coverage is 43.75%.

@@            Coverage Diff             @@
##             main    #2573      +/-   ##
==========================================
- Coverage   51.33%   51.16%   -0.17%     
==========================================
  Files         158      158              
  Lines       10512    10520       +8     
==========================================
- Hits         5396     5383      -13     
- Misses       4645     4662      +17     
- Partials      471      475       +4     
Files Changed Coverage Δ
share/p2p/discovery/discovery.go 72.45% <43.75%> (-2.99%) ⬇️

... and 4 files with indirect coverage changes

@nodersteam
Copy link
Contributor Author

@walldiss

Good afternoon
What is the timing of the review?
Or did we do PR against the rules?

walldiss

This comment was marked as duplicate.

@walldiss
Copy link
Member

Github duplicated my review, but then removed one of the copies. I've hided one, but it is still relevant. Please check it out in #2573 (review). Comments also visible from changes preview page.
Sorry for inconvenience.

@nodersteam
Copy link
Contributor Author

log.Warnf("Discovery loop is running but can't find enough peers. Found: %d, Required: %d", d.metrics.discoveryAmountOfPeers, requiredAmountOfPeers)

We can also add to the output how many peers are connected

@nodersteam
Copy link
Contributor Author

Something like this
image

Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Need to fix some nits with comments and it is good.

log.Warnf("Discovery loop is running but can't find enough peers. Found: %d, Required: %d", d.metrics.discoveryAmountOfPeers, requiredAmountOfPeers)
We can also add to the output how many peers are connected

Up to you, but if you go with this, please use d.set.Size() and d.set.Limit() values instead of what you mentioned in example

share/p2p/discovery/discovery.go Outdated Show resolved Hide resolved
share/p2p/discovery/discovery.go Outdated Show resolved Hide resolved
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

I know it feels exhausting, but we almost there! 🚀 Last nit:

share/p2p/discovery/discovery.go Outdated Show resolved Hide resolved
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Last nit

@nodersteam
Copy link
Contributor Author

I know it feels exhausting, but we almost there! 🚀 Last nit:

In fact, I would like to express my deep gratitude to you for all the comments and interactions with us here. For the first time we decided to make a contribution through interaction with the project code, and for us this is a great experience. Partly because of this, we can show that I am a little slow in understanding. It is important for us to understand exactly how interaction with development teams takes place in such large projects. Thank you very much for this, the next PR will be of better quality.

@nodersteam
Copy link
Contributor Author

Now it's look like this

image

walldiss
walldiss previously approved these changes Aug 24, 2023
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for contribution!

Wondertan
Wondertan previously approved these changes Aug 24, 2023
@Wondertan
Copy link
Member

@nodersteam, lint is failing

@Wondertan Wondertan added kind:feat Attached to feature PRs kind:chore and removed kind:chore labels Aug 24, 2023
@nodersteam
Copy link
Contributor Author

@nodersteam, lint is failing

Thank you, I found the cause, I'm already fixing it

@nodersteam nodersteam dismissed stale reviews from Wondertan and walldiss via bbb30f7 August 24, 2023 14:34
Wondertan
Wondertan previously approved these changes Aug 24, 2023
@nodersteam
Copy link
Contributor Author

@Wondertan I see that Lint is falling again, I will fix everything tomorrow and make a correct commit. I'm sorry for this

@Wondertan Wondertan enabled auto-merge (squash) August 25, 2023 12:33
@Wondertan Wondertan merged commit 4b96022 into celestiaorg:main Aug 25, 2023
13 of 15 checks passed
walldiss pushed a commit to walldiss/celestia-node that referenced this pull request Sep 22, 2023
walldiss pushed a commit that referenced this pull request Sep 22, 2023
…d wanted peers in 5 minutes (#2573)

(cherry picked from commit 4b96022)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Issues created by non node team members kind:chore kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(share/p2p/discovery): Warning for discovery loop if haven't found wanted peers in 5 minutes
5 participants