Skip to content

Store the peer list on the DB of a node#1945

Merged
mergify[bot] merged 3 commits intoCodeChain-io:masterfrom
MSNTCS:master
Feb 6, 2020
Merged

Store the peer list on the DB of a node#1945
mergify[bot] merged 3 commits intoCodeChain-io:masterfrom
MSNTCS:master

Conversation

@MSNTCS
Copy link
Contributor

@MSNTCS MSNTCS commented Jan 2, 2020

Storing all stable nodes that have been connected to the node in the database. It fixes #1569".

@MSNTCS MSNTCS changed the title Storing Peer List on the DB of the Node Store the peer list on the DB of a node Jan 3, 2020
@MSNTCS MSNTCS requested review from majecty and sgkim126 January 3, 2020 03:31
@majecty
Copy link

majecty commented Jan 3, 2020

Please add an issue number in the description of PR like "Fixes #1569".
Add a description in the commit message and PR description about why we need to save the peer list in the DB and what will be changed after this PR or commit.

Please use a full sentence in the description of PR and commit message.

@MSNTCS MSNTCS force-pushed the master branch 2 times, most recently from 34736c0 to d2dcaa7 Compare January 6, 2020 01:22
@MSNTCS MSNTCS requested a review from sgkim126 January 6, 2020 04:41
@MSNTCS
Copy link
Contributor Author

MSNTCS commented Jan 6, 2020

I tested the unpassed test( Change common Params that doesn't affect validator set) on my local machine and it passed the test. My implementation does not have any problem with this test.

@MSNTCS MSNTCS force-pushed the master branch 6 times, most recently from f609bf6 to 730b568 Compare January 6, 2020 06:46
Copy link
Contributor

@sgkim126 sgkim126 left a comment

Choose a reason for hiding this comment

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

You missed removing the address of outgoing connections.
Please check the line between 1077~1093.

@sgkim126
Copy link
Contributor

@MSNTCS Would you check the commits included in this PR again?

@MSNTCS MSNTCS force-pushed the master branch 2 times, most recently from 6141594 to 9d3f1a3 Compare January 22, 2020 02:49
@MSNTCS
Copy link
Contributor Author

MSNTCS commented Jan 29, 2020

@MSNTCS Would you check the commits included in this PR again?

Sorry, for the late response. I had changed when I saw your comment. Now is only related commit.

@MSNTCS MSNTCS requested a review from sgkim126 January 29, 2020 01:44
@MSNTCS MSNTCS requested a review from majecty February 5, 2020 06:08
@MSNTCS MSNTCS force-pushed the master branch 3 times, most recently from 219506c to f0ce9b4 Compare February 5, 2020 07:29
@majecty
Copy link

majecty commented Feb 5, 2020

Please run rustfmt whenever you create a commit.

@majecty
Copy link

majecty commented Feb 5, 2020

Your commit messages "Update handler.rs" and "Update service.rs" does not give any meaningful information to a reader.
It seems that the reason why you created the two commits is that some code is different from Foundry's and yours. Then could you give the information in the commit message?

@majecty
Copy link

majecty commented Feb 5, 2020

@MSNTCS Could you also cherry pick these commits?
IMO, you can squash them in your first commit.
CodeChain-io/foundry@e22ea52
CodeChain-io/foundry@c6dcf54
CodeChain-io/foundry@82716dd
CodeChain-io/foundry@7471a90
CodeChain-io/foundry@d530645
CodeChain-io/foundry@97b8c62

Never mind the above message. I misunderstood this PR. Sorry.

@MSNTCS MSNTCS force-pushed the master branch 6 times, most recently from 6dc21fb to e40d00e Compare February 5, 2020 11:12
sgkim126
sgkim126 previously approved these changes Feb 6, 2020
Copy link
Contributor

@sgkim126 sgkim126 left a comment

Choose a reason for hiding this comment

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

LGTM.
Let's merge it after @majecty 's approval.

@MSNTCS MSNTCS requested a review from majecty February 6, 2020 02:40
use crate::routing_table::RoutingTable;
use crate::{p2p, Api, NetworkExtension, SocketAddr};
use crate::{p2p, Api, ManagingPeerdb, NetworkExtension, SocketAddr};

Copy link

Choose a reason for hiding this comment

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

Please remove this empty line.

To reduce the burden of bootstrap nodes, codecahin needs to save the received
peer list  in the disk. In order to save the peer list of a node in the disk, a
procedure is designed a database to store nodes that are connected to the node.
Copy link

@majecty majecty left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@MSNTCS MSNTCS requested a review from sgkim126 February 6, 2020 04:18
@mergify mergify bot merged commit 4e9b29d into CodeChain-io:master Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Save the peer list on the disk

3 participants