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: use deferred initialization of the asnStore #3

Merged
merged 2 commits into from
Aug 25, 2020

Conversation

aschmahmann
Copy link
Contributor

@aschmahmann aschmahmann commented Aug 25, 2020

This is meant to deal with the issue where initialization of the data structure by loading in 50k entries adds a bunch of time to startup. While we should be using better data structures here + serializing/deserializing the structured (instead of unstructured) ASN map at runtime this should at least allow this library to be used async without slowing down the main thread.

Comment on lines +87 to +88
// If no mapping exists for the given IP, this function will
// return an empty ASN and a nil error.
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a default cidr prefix for unknown mappings?

Copy link
Contributor Author

@aschmahmann aschmahmann Aug 25, 2020

Choose a reason for hiding this comment

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

Can you elaborate a little more? I'm not understanding the question

Copy link
Contributor

Choose a reason for hiding this comment

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

if 2 ips in the same /64 are looked up in a space that isn't in the mapping, will they get the same empty ASN, or different ones?
when we make a new ASN in response to a query, we should maybe save it in the mapping with some default prefix so that other IPs close to it cluster with it.

Copy link
Contributor Author

@aschmahmann aschmahmann Aug 25, 2020

Choose a reason for hiding this comment

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

will they get the same empty ASN, or different ones?

We return "" and when this is used in kbucket we just reject ASNs named "".

Note, this is why we're going to have to change your test in go-ipfs to use an ipv6 associated with a real ASN https://github.com/ipfs/go-ipfs/blob/fe9f49aeb71c753b92ed41922c493ff3bf08ef56/test/integration/wan_lan_dht_test.go#L54

@aschmahmann aschmahmann requested a review from petar August 25, 2020 16:17
asn.go Outdated Show resolved Hide resolved
asn.go Outdated Show resolved Hide resolved
@aschmahmann aschmahmann requested a review from petar August 25, 2020 19:14
@aschmahmann aschmahmann merged commit 660f020 into feat/init Aug 25, 2020
@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
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.

3 participants