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

feat: kademlia blocklists peer on ping timeout #2543

Merged
merged 8 commits into from
Sep 29, 2021
Merged

Conversation

istae
Copy link
Member

@istae istae commented Sep 27, 2021

This change is Reviewable

@istae istae requested review from mrekucci and acud September 27, 2021 13:02
@istae istae requested a review from notanatol September 27, 2021 13:02
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 4 files at r1, 1 of 1 files at r2.
Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @acud, @mrekucci, and @notanatol)


pkg/blocker/blockler.go, line 17 at r2 (raw file):

type peer struct {
	flagged    bool      // indicates whether the peer is actively flagged

You can get rid of the flagged field. Instead of this, you can use time zero value as a sentinel and as a result, the code will have more clarity.


pkg/blocker/blockler.go, line 65 at r2 (raw file):
As a first iteration this is good, just keep in mind that it can be quite memory intensive, see the documentation:

The underlying Timer is not recovered by the garbage collector
until the timer fires. If efficiency is a concern, use NewTimer
instead and call Timer.Stop if the timer is no longer needed.


pkg/p2p/p2p.go, line 38 at r2 (raw file):

type Blocklister interface {
	// Blocklist will disconnect a peer and put it on a blocklist (blocking in & out connections) for provided duration

Missing period at the end of the sentence. The duration on the line below should be capitalized.

@mrekucci mrekucci self-requested a review September 27, 2021 14:15
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 3 files at r3, 4 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @acud)


pkg/blocker/blockler.go, line 22 at r5 (raw file):

var (
	wakeupTime time.Duration = time.Second * 10

This can be a constant.

@istae istae added the ready for review The PR is ready to be reviewed label Sep 27, 2021
Copy link
Member

@acud acud 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 3 files at r3, 3 of 4 files at r5.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @acud, @istae, and @mrekucci)


pkg/blocker/blockler.go, line 5 at r5 (raw file):

// license that can be found in the LICENSE file.

package blocker

file needs to be renamed


pkg/blocker/blockler.go, line 107 at r5 (raw file):

// must be called only once.
func (b *Blocker) Close() error {
	b.quit <- struct{}{}

it is better to close the channel since a naked write can block for a long time. also, since the worker goroutine might need to use the disconnecter interface even after the channel is closed, resulting in a panic (this is not the case right now, because the naked right makes sure that the synchronization happens in the correct way).

Copy link
Contributor

@notanatol notanatol left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @acud, @istae, and @mrekucci)


pkg/blocker/blockler.go, line 22 at r5 (raw file):

Previously, mrekucci wrote…

This can be a constant.

it's being overwritten in the export_test.go for the purpose of testing.

pkg/blocker/blocker.go Show resolved Hide resolved
pkg/blocker/blocker.go Show resolved Hide resolved
@sonarqubecloud
Copy link

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 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@istae istae merged commit c378bfa into master Sep 29, 2021
@istae istae deleted the block-on-ping-err branch September 29, 2021 09:36
@acud acud added this to the v1.2.0 milestone Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants