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

refactor: wrap peer store #3051

Merged
merged 15 commits into from
Sep 27, 2024
Merged

refactor: wrap peer store #3051

merged 15 commits into from
Sep 27, 2024

Conversation

darshankabariya
Copy link
Contributor

@darshankabariya darshankabariya commented Sep 23, 2024 •
edited
Loading

Description

Refactored the peer manager to improve stale peer management.

Changes

  • [ abstract the peer_store functionality by using waku_peer_store] --> This allows for extending features in waku_peer_store without modifying the core peer_store.
  • [ Improved WakuPeerStore: unified peer data, enhanced queries, and added Waku-specific features for better management. ]

@darshankabariya darshankabariya changed the title Refactor Peermanager for stale peers. Refactor: peermanager for stale peers management. Sep 23, 2024
@darshankabariya darshankabariya changed the title Refactor: peermanager for stale peers management. refactor: peermanager for stale peers management. Sep 23, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 23, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3051

Built from 69a20a5

@darshankabariya darshankabariya marked this pull request as draft September 24, 2024 05:27
@darshankabariya darshankabariya force-pushed the refactor_peermanager branch 2 times, most recently from 20cd4f0 to 63797a9 Compare September 25, 2024 12:43
@darshankabariya darshankabariya changed the title refactor: peermanager for stale peers management. refactor: wrap peer store Sep 26, 2024
@darshankabariya darshankabariya marked this pull request as ready for review September 27, 2024 05:57
Ivansete-status
Ivansete-status requested changes Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Great PR! Thanks so much for it! 🥳

I've just suggested some minor changes.
On the other hand, I have the impression that we are encapsulating the code a little, which is great indeed, but we are not yet tackling the stale peers issue, isn't it?

waku/node/peer_manager/peer_manager.nim Outdated
@@ -73,7 +73,7 @@ const

type PeerManager* = ref object of RootObj
switch*: Switch
peerStore*: PeerStore
peerStore*: WakuPeerStore
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should also remane the variable name. Maybe do the same in the tests

Suggested change
peerStore*: WakuPeerStore
wakuPeerStore*: WakuPeerStore

waku/node/peer_manager/waku_peer_store.nim Outdated
@@ -16,14 +16,16 @@ import
export peerstore, builders

type
WakuPeerStore* = ref object
store*: PeerStore
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a great opportunity to make the field private. In the suggestion I'm also renaming the attribute :)

Suggested change
store*: PeerStore
peerStore: PeerStore

waku/node/peer_manager/waku_peer_store.nim Outdated
proc connectedness*(peerStore: PeerStore, peerId: PeerID): Connectedness =
peerStore[ConnectionBook].book.getOrDefault(peerId, NotConnected)
proc connectedness*(wps: WakuPeerStore, peerId: PeerId): Connectedness =
wps.store[ConnectionBook].book.getOrDefault(peerId, NotConnected)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if would it work the following, for consistency

Suggested change
wps.store[ConnectionBook].book.getOrDefault(peerId, NotConnected)
wps[ConnectionBook].book.getOrDefault(peerId, NotConnected)

waku/node/peer_manager/waku_peer_store.nim

# TODO: Rename peers() to getPeersByProtocol()
proc peers*(peerStore: PeerStore): seq[RemotePeerInfo] =
## Get all the stored information of every peer.
proc peers*(wps: WakuPeerStore): seq[RemotePeerInfo] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should also remane those to getPeers or getWakuPeers to make them more explicit and easier to lookup

@darshankabariya
Copy link
Contributor Author

Great PR! Thanks so much for it! 🥳

I've just suggested some minor changes. On the other hand, I have the impression that we are encapsulating the code a little, which is great indeed, but we are not yet tackling the stale peers issue, isn't it?

Yes, this PR does not tackle the stale peer issues. That stale peer issue isn't as pressing as it seemed when I created it. That's why I focused on another aspect of refactoring. Regarding the stale peer issue, I will add another PR to separate the functionality of the peer manager and the connection lifecycle. In that, I will also improve stale peer management.

gabrielmer
gabrielmer approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@gabrielmer gabrielmer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much!

@darshankabariya darshankabariya merged commit 729e63f into master Sep 27, 2024
9 of 11 checks passed
@darshankabariya darshankabariya deleted the refactor_peermanager branch September 27, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@gabrielmer gabrielmer gabrielmer approved these changes

@SionoiS SionoiS Awaiting requested review from SionoiS

@NagyZoltanPeter NagyZoltanPeter Awaiting requested review from NagyZoltanPeter

Assignees

@darshankabariya darshankabariya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@darshankabariya @gabrielmer @Ivansete-status