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

Improve ObservedAddrSet behaviour #191

Merged
merged 3 commits into from
Apr 24, 2017
Merged

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Apr 22, 2017

No description provided.

@@ -8,15 +8,32 @@ import (
ma "github.com/multiformats/go-multiaddr"
)

const ActivationTrsh = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Trsh/Thresh/ Looks less like 'Trash'

// ObservedAddr is an entry for an address reported by our peers.
// We only use addresses that:
// - have been observed more than once. (counter symmetric nats)
// - have been observed recently (10min), because our position in the
// - have been observed more than 3 times in last 10 min. (counter symmetric nats)
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be more than ten minutes I think. Can probably get away with making it an hour

Copy link
Member Author

Choose a reason for hiding this comment

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

I would extend the time of the aspect explained lower. After it was observed and confirmed (4 observations in 10 min duration) keep it as valid address for an hour. How does that sound?

Copy link
Member Author

Choose a reason for hiding this comment

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

And it is an hour that is extended every time address is observed by anyone again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the four observations should be able to occur over a longer timespan than ten minutes.

Copy link
Contributor

Choose a reason for hiding this comment

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

update comment

ma "github.com/multiformats/go-multiaddr"
)

const (
ActivationTresh = 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Close, s/Tresh/Thresh

@@ -69,7 +80,7 @@ func (oas *ObservedAddrSet) Add(addr ma.Multiaddr, observer ma.Multiaddr) {
// for zero-value.
if oas.addrs == nil {
oas.addrs = make(map[string]*ObservedAddr)
oas.ttl = pstore.OwnObservedAddrTTL
oas.ttl = OwnObservedAddrTTL
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure we want to disconnect this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched back and opened. libp2p/go-libp2p-peerstore#13

Copy link
Contributor

@whyrusleeping whyrusleeping 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, a few comments then good to go

@whyrusleeping
Copy link
Contributor

also tests are failing

@Kubuxu Kubuxu force-pushed the feat/identify/smarter-obsaddrs branch 5 times, most recently from 0ba432b to 0dee2f4 Compare April 23, 2017 11:45
Copy link
Contributor

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM (make sure the gx hash updated here is of publishing the latest commit)

@Kubuxu
Copy link
Member Author

Kubuxu commented Apr 24, 2017

Oh, there shouldn't be hash update here. It slipped in.

@Kubuxu Kubuxu force-pushed the feat/identify/smarter-obsaddrs branch from 2e966e2 to ed98e37 Compare April 24, 2017 18:26
@Kubuxu Kubuxu merged commit d8565dd into master Apr 24, 2017
@Kubuxu Kubuxu removed status/in-progress In progress labels Apr 24, 2017
@Kubuxu Kubuxu deleted the feat/identify/smarter-obsaddrs branch April 24, 2017 21:01
marten-seemann pushed a commit that referenced this pull request Apr 21, 2022
change backoffs to per-address
marten-seemann added a commit that referenced this pull request Aug 9, 2022
optimize allocations in the memory address book
marten-seemann added a commit that referenced this pull request Aug 17, 2022
* run go mod tidy

* omit receiver name if unused

* remove unused type testkey in tests

* fix duplicate import of go-multiaddr

* fix use of deprecated peer.IDB58{Encode,Decode}

* use bytes.Equal instead of bytes.Compare

* fix unnecessary assigments to blank identifier

* use time.Until instead of t.Sub(time.Now())

* fix use of deprecated go-multihash.ID

* add missing error check in envelope test

* fix error check in tests
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.

None yet

2 participants