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(discovery)!: discover peers by tag #2730

Merged
merged 8 commits into from
Oct 9, 2023

Conversation

walldiss
Copy link
Member

Allow to run multiple discovery subcomponents, where each can find new peers based on preset tag and notify its subscribers. It could allow any sort of separate discovery subcomponents (versioned, pruned/archived, etc). Essentially it allows discovery to be abstracted away and act as single function component.

Involves some minor refactoring here and there just to clean up things.

Resolves #2578

@walldiss walldiss self-assigned this Sep 19, 2023
@walldiss walldiss added kind:feat Attached to feature PRs area:p2p labels Sep 19, 2023
ramin
ramin previously approved these changes Sep 19, 2023
Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

only basic understanding of how this all works so take my review with grain of salt, but beautiful ❤️

share/p2p/discovery/discovery_test.go Show resolved Hide resolved
share/p2p/discovery/discovery.go Show resolved Hide resolved
# Conflicts:
#	share/p2p/discovery/options.go
@walldiss walldiss marked this pull request as ready for review September 25, 2023 14:35
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2023

Codecov Report

Merging #2730 (40c4b6c) into main (8602d6e) will decrease coverage by 0.24%.
Report is 21 commits behind head on main.
The diff coverage is 68.96%.

@@            Coverage Diff             @@
##             main    #2730      +/-   ##
==========================================
- Coverage   51.73%   51.49%   -0.24%     
==========================================
  Files         161      167       +6     
  Lines       10826    10758      -68     
==========================================
- Hits         5601     5540      -61     
- Misses       4731     4737       +6     
+ Partials      494      481      -13     
Files Coverage Δ
nodebuilder/share/config.go 0.00% <ø> (ø)
nodebuilder/share/module.go 0.00% <ø> (ø)
share/availability/full/testing.go 100.00% <100.00%> (ø)
share/p2p/discovery/metrics.go 11.53% <0.00%> (ø)
share/p2p/discovery/discovery.go 76.47% <88.88%> (+3.58%) ⬆️
share/p2p/discovery/options.go 74.07% <76.47%> (+14.98%) ⬆️
nodebuilder/share/constructors.go 9.80% <0.00%> (-0.20%) ⬇️
share/p2p/peers/manager.go 71.92% <45.45%> (-0.27%) ⬇️

... and 26 files with indirect coverage changes

share/p2p/discovery/options.go Outdated Show resolved Hide resolved
nodebuilder/share/constructors.go Show resolved Hide resolved
renaynay
renaynay previously approved these changes Sep 29, 2023
share/p2p/discovery/discovery_test.go Outdated Show resolved Hide resolved
share/p2p/peers/manager.go Outdated Show resolved Hide resolved
distractedm1nd
distractedm1nd previously approved these changes Oct 5, 2023
@walldiss walldiss dismissed stale reviews from distractedm1nd and renaynay via 40c4b6c October 6, 2023 07:26
distractedm1nd
distractedm1nd previously approved these changes Oct 9, 2023
@distractedm1nd
Copy link
Collaborator

i didnt know i could double approve like that lol

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Ok, so this is pretty simple. For multiple discoveries we will need something like DiscoveryManager for Discovery CRUD

share/p2p/discovery/options.go Outdated Show resolved Hide resolved
- fix params validation
- update test to use Start/Advertise api instead of passing special values params
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Making tag required makes sense

@walldiss walldiss merged commit ec4472a into celestiaorg:main Oct 9, 2023
14 of 16 checks passed
@renaynay renaynay changed the title feat(discovery): discover peers by tag feat(discovery)!: discover peers by tag Oct 11, 2023
@renaynay renaynay added the kind:break! Attached to breaking PRs label Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:p2p kind:break! Attached to breaking PRs kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

share/discovery: Implement versioning for full advertising string
6 participants