network: allow multi-role phonebook entries #6131
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6131 +/- ##
==========================================
+ Coverage 56.19% 56.24% +0.04%
==========================================
Files 494 494
Lines 69943 69971 +28
==========================================
+ Hits 39307 39352 +45
+ Misses 27960 27952 -8
+ Partials 2676 2667 -9 ☔ View full report in Codecov by Sentry. |
algorandskiy
changed the title
| // currently, we have two roles : relay role and archival role, which are mutually exclusive. | ||
| // | ||
| //msgp:ignore PhoneBookEntryRoles | ||
| type PhoneBookEntryRoles int |
There was a problem hiding this comment.
renamed because of linter complaint about "phonebook.PhoneBookXYZ" repeating
| @@ -782,7 +782,7 @@ func TestLocal_ValidateP2PHybridConfig(t *testing.T) { | |||
| NetAddress: test.netAddress, | |||
| } | |||
| err := c.ValidateP2PHybridConfig() | |||
| require.Equal(t, test.err, err != nil, "test=%d", i) | |||
There was a problem hiding this comment.
another linter complaint
|
|
||
| // also discover archival nodes | ||
| var dhtArchivalPeers []peer.AddrInfo | ||
| const numPeersToDiscover = 5 // some arbitrary number TODO: figure out a better value based on peerSelector/fetcher algorithm |
There was a problem hiding this comment.
out of curiosity why 5 vs 4 (or vs GossipFanout)?
There was a problem hiding this comment.
Maybe put this const at the top of the file?
| if peerCore, ok := addrInfoToWsPeerCore(n, &info); ok { | ||
| peers = append(peers, &peerCore) | ||
| } | ||
| const maxNodes = 5 // some arbitrary number |
There was a problem hiding this comment.
Should this be the same global const as numPeersToDiscover like numArchivePeersToDiscover at the top
| @@ -163,15 +194,20 @@ func (e *phonebookImpl) filterRetryTime(t time.Time, role PhoneBookEntryRoles) [ | |||
| // new entries in addressesThey are being added | |||
| // existing items that aren't included in addressesThey are being removed | |||
| // matching entries don't change | |||
| func (e *phonebookImpl) ReplacePeerList(addressesThey []string, networkName string, role PhoneBookEntryRoles) { | |||
| func (e *phonebookImpl) ReplacePeerList(addressesThey []string, networkName string, role Roles) { | |||
There was a problem hiding this comment.
so phonebook.go's phonebookImpl.ReplacePeerList and peerstore.go's PeerStore.ReplacePeerList are the same?
Summary
meshThreadfromGetPeers:GeetPeersbe non-network query dependentTest Plan
Added unit tests for multi roles peers in phonebook and peerstore wrapper.