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: warn users who are falling behind reprovides #9886

Merged
merged 3 commits into from
Jun 8, 2023

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented May 19, 2023

Fixes: #9704
Fixes: #9702
Fixes: #9703
Fixes: #9419

@Jorropo Jorropo requested a review from a team as a code owner May 19, 2023 18:25
@BigLep
Copy link
Contributor

BigLep commented May 23, 2023

2023-05-23 conversation: @Jorropo will pull out the go-ipfs-http-client into a separate PR and link it to #9124

Copy link
Contributor

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @Jorropo . I just focused on the user documentation side of this.

I see this issue as also intending to close #9703 (I'll update the PR description), but that means addressing #9419 as well. Can you include the relevant doc suggestions from there as well?

docs/changelogs/v0.21.md Outdated Show resolved Hide resolved
docs/changelogs/v0.21.md Outdated Show resolved Hide resolved
docs/changelogs/v0.21.md Outdated Show resolved Hide resolved
docs/changelogs/v0.21.md Outdated Show resolved Hide resolved
docs/config.md Outdated
Comment on lines 1404 to 1431
Utilizes an alternative DHT client that searches for and maintains more information
about the network in exchange for being more performant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Utilizes an alternative DHT client that searches for and maintains more information
about the network in exchange for being more performant.
Utilizes an alternative DHT client that searches for and maintains more information
about the network in exchange for improved provide latency performance (upwards of 100x).

(I made up the number above - I can't remember the actual numbers. Maybe they are in the changelog for when we first released this?)

Can we maybe expand this by a sentence or two? Is there a talk or doc we can link to?

Maybe something along the lines of...

This accelerated DHT client builds up a bamp of various nodes that can satisfy the keyspace.  This helps both with reads and writes:
* Reads - Traversed DHT results have been cached and know the closest nodes to query.
* Writes - In addition to having the cached traversed DHT results to know the direct n=20 nodes to store records with, records are batched so that do a single write with a large batch of records for a given provider.

(I know that's not a great writeup - but would like to have some more text that helps someone conceptually understand what is going on, or provide a link where someone can learn more so it's more self-service to reason about the system.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess some of these details are covered below in the "when it is enabled" section. I mostly just want to make sure we help explain the concept of what's happening.

Copy link
Contributor Author

@Jorropo Jorropo Jun 1, 2023

Choose a reason for hiding this comment

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

I'll rework that section in the docs, I don't think changelogs should go in that much details, just link to docs that are that detailed.

That is not the changelog.

Copy link
Contributor Author

@Jorropo Jorropo Jun 2, 2023

Choose a reason for hiding this comment

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

Btw the number is roughly 6 millions. 6 millions times faster.
(based on the assumption of 30s per bucket rt puts, vs 5µs for an accelerated swept put)

Copy link
Contributor

Choose a reason for hiding this comment

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

At what percentile are we getting 30s per bucket round trip? That seems really high. I assume the p50 value is way lower, and we should compare against that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I belive that for all 20 provides, because almost all the time one of them will timeout and the timeout is 30s ?
Anyway 5µs is so small even if puts took 250ms this would still be way faster.

core/node/provider.go Outdated Show resolved Hide resolved
core/node/provider.go Outdated Show resolved Hide resolved
core/node/provider.go Show resolved Hide resolved
}

// How long per block that lasts us.
expectedProvideSpeed := reprovideInterval / time.Duration(count)
Copy link
Contributor

Choose a reason for hiding this comment

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

What unit of time are we dealing with here? Seconds, milliseconds? I think for clarity, it would be good to embed that into the variable names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The computations are done in nanoseconds, time.Duration.String takes care of formating it in something relevent.

Go use a typed time approach, programmers do not have to care about the unit of time, they do operations on the time.Duration time (which internally use nanoseconds) and then various formating call let extract the precision wanted.

core/node/provider.go Show resolved Hide resolved
@BigLep
Copy link
Contributor

BigLep commented May 30, 2023

I added:

Fixes: #9814
Fixes: #9703

@Jorropo
Copy link
Contributor Author

Jorropo commented May 31, 2023

@BigLep you don't want to use

Fixes: #9814

Fixes precisely describe that the issue following will be closed when this is merged. Updates: #9814 or For: #9814 would be appropriate. 🙂

@BigLep
Copy link
Contributor

BigLep commented May 31, 2023

@Jorropo : thanks. I screwed up my copy paste. I meant to do:

Fixes: #9419
Fixes: #9703

I didn't mean to mention #9814! Arg!

Anyways, fixed now!

@BigLep BigLep requested a review from hacdias June 1, 2023 01:16
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase, check @BigLep's review. Sharness seems to have timed out though.

docs/config.md Outdated Show resolved Hide resolved
Copy link
Contributor

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

I left comments on config.md. I am happy to take another look if it helps, but I don't have any blockers here.

Thanks for handling!

The [accelerated DHT client](docs/config.md#routingaccelerateddhtclient) is now
the main recommended solution for users who are hosting lots of data.
By trading some upfront DHT caching and increased memory usage,
one gets provider throughput improvements up to 6 millions times bigger dataset.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
one gets provider throughput improvements up to 6 millions times bigger dataset.
one gets provider throughput improvements of up to 6 million times on larger datasets!

docs/config.md Outdated
Comment on lines 1404 to 1431
Utilizes an alternative DHT client that searches for and maintains more information
about the network in exchange for being more performant.
Copy link
Contributor

Choose a reason for hiding this comment

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

At what percentile are we getting 30s per bucket round trip? That seems really high. I assume the p50 value is way lower, and we should compare against that.

docs/config.md Outdated
- DHT operations should complete much faster than with it disabled
- A batching reprovider system will be enabled which takes advantage of some
properties of the experimental client to very efficiently put provider records into the network
- The standard DHT client (and server if enabled) are run alongside the accelerated DHT client
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following everything here @Jorropo .

When the AcceleratedDHTClient is enabled:

  1. (Client) For both writing records and reading records, we use the full routing table right?
  2. (Server) Storing records on behalf of others and responding to queries, we use the bucket routing table right?

- A batching reprovider system will be enabled which takes advantage of some
properties of the experimental client to very efficiently put provider records into the network
- The standard DHT client (and server if enabled) are run alongside the accelerated DHT client
- The operations `ipfs stats dht` will default to showing information about the accelerated DHT client
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Thinking about this more, I'm wondering if we even need this comment.

A user has these options:

ipfs stats dht wanserver
ipfs stats dht lanserver
ipfs stats dht wan
ipfs stats dht lan

When the accelerated DHT client is enabled, that means its stats will be reported for ipfs stats dht wan right?

And ipfs stats dht is just shorthand for ipfs stats dht wan right?

I'm trying to understand the purpose of this comment. I don't think we should be capturing here what ipfs stats dht defaults to showing. I do think we should say that accelerated DHT client affects dht wan, but the other 3 are untouched.

core/node/provider.go Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
@Jorropo Jorropo force-pushed the accelerated-dht-client branch 7 times, most recently from bcbd21e to 93c57dc Compare June 8, 2023 07:46
We were quite inconsistent about this previously, some files used 1.19.1 some 1.19.x, this makes it more consistent.
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM. Assuming everything passes, please merge.

@Jorropo Jorropo merged commit e7294cb into ipfs:master Jun 8, 2023
@Jorropo Jorropo deleted the accelerated-dht-client branch June 8, 2023 08:05
@lidel lidel mentioned this pull request Sep 15, 2023
@lidel lidel mentioned this pull request Nov 24, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
3 participants