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: removed kademlia and libp2p discrepancy kludge #2429

Merged
merged 3 commits into from
Sep 1, 2021
Merged

Conversation

istae
Copy link
Member

@istae istae commented Aug 19, 2021

After running a bee node in main-net for two days with the discrepancy metric, it was discovered that the issue has been fixed and the kludge can be removed.

PR includes deepsource fixes:
kad.connected renamed to kad.onConnected
pslice.exists renamed to pslice.index
Announce, Connected, AnnounceTo split into two new functions to avoid control coupling or simplified

This change is Reviewable

@acud
Copy link
Member

acud commented Aug 19, 2021

should we PR this to master? maybe running a bunch of nodes in a custom testnet/mainnet cluster would do the trick, so we can PR and fix the bug in one go?

Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @istae)


pkg/topology/kademlia/metrics.go, line 137 at r1 (raw file):

			Namespace: m.Namespace,
			Subsystem: subsystem,
			Name:      "p2p_kademlia_discrepancy",

I'd remove the kademlia from the Name since it is already explicitly stated in the subsystem variable.


pkg/topology/kademlia/metrics.go, line 138 at r1 (raw file):

			Subsystem: subsystem,
			Name:      "p2p_kademlia_discrepancy",
			Help:      "Discrepancy counter between lists of peers in kademlia and libp2p",

I'd also remove the counter from the description, since it is implicit. Maybe The discrepancy between lists of peers in kademlia and libp2p.

@istae istae changed the title chore: kademlia and libp2p discrepancy metric fix: removed kademlia and libp2p discrepancy kludge Aug 25, 2021
Copy link
Contributor

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @istae)

@istae istae requested review from acud and metacertain August 31, 2021 11:30
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@istae istae merged commit c778dad into master Sep 1, 2021
@istae istae deleted the p2p-kad-discrep branch September 1, 2021 11:09
@acud acud added this to the v1.2.0 milestone Sep 30, 2021
acud added a commit that referenced this pull request Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants