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

fix: Add reconnection to known peers #1482

Merged

Conversation

fredcarle
Copy link
Collaborator

@fredcarle fredcarle commented May 10, 2023

Relevant issue(s)

Resolves #1481

Description

Currently, we de not reconnect to known peers on restart unless the peer is defined in the config file or on the CLI command. This PR makes sure that we try to reconnect to known peers.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test shows now degradation and tested manually that peers reconnect on restart.

Specify the platform(s) on which this was tested:

  • MacOS

@fredcarle fredcarle added the area/p2p Related to the p2p networking system label May 10, 2023
@fredcarle fredcarle added this to the DefraDB v0.5.1 milestone May 10, 2023
@fredcarle fredcarle requested a review from a team May 10, 2023 16:16
@fredcarle fredcarle self-assigned this May 10, 2023
@fredcarle fredcarle force-pushed the fredcarle/fix/restart-peers branch from 1f42c68 to 3cb29dc Compare May 10, 2023 16:23
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #1482 (3cb29dc) into develop (7463448) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 3cb29dc differs from pull request most recent head 479f1d7. Consider uploading reports for the commit 479f1d7 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1482      +/-   ##
===========================================
- Coverage    72.21%   72.20%   -0.01%     
===========================================
  Files          185      185              
  Lines        18226    18238      +12     
===========================================
+ Hits         13161    13168       +7     
- Misses        4027     4030       +3     
- Partials      1038     1040       +2     
Impacted Files Coverage Δ
net/peer.go 51.28% <100.00%> (+0.95%) ⬆️

... and 5 files with indirect coverage changes

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Approving so long as you test this manually first, as I don't have the bandwidth to finish of the restart tests atm, and make test (as stated in the PR description) will not test that this works atm.

net/peer.go Outdated Show resolved Hide resolved
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

A todo, and suggestion. Similar to Andy haven't manually tested. Giving an LGTM

@fredcarle fredcarle force-pushed the fredcarle/fix/restart-peers branch from 3cb29dc to 479f1d7 Compare May 10, 2023 18:26
@fredcarle fredcarle merged commit 36261e1 into sourcenetwork:develop May 10, 2023
@fredcarle fredcarle deleted the fredcarle/fix/restart-peers branch May 10, 2023 18:36
AndrewSisley added a commit that referenced this pull request May 15, 2023
## Relevant issue(s)

Resolves #1082 

## Description

Adds DB/Node Restart tests. This will be required by Lens (e.g. to make
sure migrations are not lost on restart). IIRC I think this also found
#1482 (fixed by Fred).

---------

Co-authored-by: Fred Carle <[email protected]>
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1481 

## Description

Currently, we de not reconnect to known peers on restart unless the peer
is defined in the config file or on the CLI command. This PR makes sure
that we try to reconnect to known peers.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
## Relevant issue(s)

Resolves sourcenetwork#1082 

## Description

Adds DB/Node Restart tests. This will be required by Lens (e.g. to make
sure migrations are not lost on restart). IIRC I think this also found
sourcenetwork#1482 (fixed by Fred).

---------

Co-authored-by: Fred Carle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/p2p Related to the p2p networking system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconnect to known peers on restart
3 participants