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 checkMaxConnections #3126

Merged
merged 6 commits into from
Nov 26, 2019
Merged

Refactor checkMaxConnections #3126

merged 6 commits into from
Nov 26, 2019

Conversation

devinbileck
Copy link
Member

Fix connection limit checks so as to prevent the following warning:

WARN b.n.p2p.peers.PeerManager: No candidates found to remove (That
case should not be possible as we use in the last case all
connections).

@devinbileck devinbileck requested a review from ripcurlx as a code owner August 22, 2019 07:22
@mrosseel
Copy link
Contributor

this might be a good opportunity to add a unit test for PeerManager (possibly limited to the scope of connection handling)

ripcurlx
ripcurlx previously approved these changes Aug 22, 2019
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK

@devinbileck
Copy link
Member Author

this might be a good opportunity to add a unit test for PeerManager (possibly limited to the scope of connection handling)

It seems the PeerManagerTest is ignored and in need of an overhaul.

@ripcurlx
Copy link
Contributor

Could someone of the exisiting contributors try out this branch before merging, as it would cause big problems if some use-case handling is wrong. I just read through the code and it looks fine, but it would be good to give this a local test as well.

@ManfredKarrer
Copy link
Contributor

I think that code part is really a good candidate for a test. The logic is a bit involved and bigger changes here are tough to test manually. I would postpone otherwise merge until someone had dedicated enough time to be sure all is correct.

@devinbileck
Copy link
Member Author

Agreed. I will try to look into implementing some tests. Just not familiar with this area so will need some time to ramp up.

@ripcurlx
Copy link
Contributor

Agreed. I will try to look into implementing some tests. Just not familiar with this area so will need some time to ramp up.

@devinbileck Are you still on this PR?

@devinbileck
Copy link
Member Author

I started looking into it but didn't make much progress. I haven't focused on it very much lately. If someone else is able to contribute something, feel free. Otherwise I will try to get to it.

@ripcurlx ripcurlx mentioned this pull request Oct 7, 2019
Closed
@devinbileck devinbileck mentioned this pull request Oct 9, 2019
Closed
@devinbileck devinbileck mentioned this pull request Nov 11, 2019
Closed
@devinbileck
Refactor checkMaxConnections
6b22690 
Fix connection limit checks so as to prevent the following warning:

> WARN  b.n.p2p.peers.PeerManager: No candidates found to remove (That
case should not be possible as we use in the last case all
connections).
@devinbileck
Change access level for checkMaxConnections to be tested
22e93d9
@ripcurlx
Copy link
Contributor

@devinbileck Could you please resolve the conflict and add some tests?
@freimair Could you review the code?

@devinbileck
Copy link
Member Author

I have added some tests. I will be committing them shortly.

@devinbileck devinbileck force-pushed the refactor-check-max-connections branch from 18e4190 to 500a90a Compare November 13, 2019 07:21
@devinbileck
Add PeerManagerTest
b45765f 
The old PeerManagerTest was located under network/p2p/routing, which is
no longer the correct location. Additionally, it was outdated so I
just removed it and added a new file under network/p2p/peers containing
tests for checkMaxConnections.
@devinbileck devinbileck force-pushed the refactor-check-max-connections branch from 500a90a to b45765f Compare November 13, 2019 07:31
@devinbileck
Add testCompile dependency to core
21572ea 
This is necessary because bisq.network.p2p.MockNode imports
bisq.core.network.p2p.seed.DefaultSeedNodeRepository.
@devinbileck
Copy link
Member Author

@ripcurlx @freimair ready for review.

@ripcurlx ripcurlx requested a review from freimair November 14, 2019 14:48
@ripcurlx
Copy link
Contributor

@freimair As I haven't touched this part of the codebase so far, could you please do the review? Thanks!

freimair
freimair suggested changes Nov 18, 2019
View reviewed changes
Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

IMO we could get away with mocking the SeedNodeRepository superclass, thus eliminating the dependency to core. Can you verify that this is correct?

build.gradle Outdated Show resolved Hide resolved
p2p/src/test/java/bisq/network/p2p/MockNode.java Outdated Show resolved Hide resolved
p2p/src/test/java/bisq/network/p2p/MockNode.java Outdated Show resolved Hide resolved
@devinbileck
Update based on review feedback
e99d450 
Mock the SeedNodeRepository superclass, thus eliminating the dependency
to core.
@devinbileck
Copy link
Member Author

Yes, good point. Updated!

@ripcurlx ripcurlx merged commit 9c3c618 into bisq-network:master Nov 26, 2019
@devinbileck devinbileck deleted the refactor-check-max-connections branch November 26, 2019 18:50
@devinbileck devinbileck mentioned this pull request Dec 13, 2019
Closed
@ripcurlx ripcurlx mentioned this pull request Dec 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freimair freimair freimair approved these changes

@ripcurlx ripcurlx ripcurlx left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devinbileck @mrosseel @ripcurlx @ManfredKarrer @freimair