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

f/optimistic provide #1014

Merged
merged 1 commit into from
Dec 12, 2023
Merged

f/optimistic provide #1014

merged 1 commit into from
Dec 12, 2023

Conversation

altergui
Copy link
Contributor

@altergui altergui commented Jun 26, 2023

fix #989

@altergui altergui requested a review from p4u June 26, 2023 06:54
@altergui altergui force-pushed the f/optimistic-provide branch 2 times, most recently from 815375c to eff10e1 Compare June 26, 2023 08:39
@p4u
Copy link
Member

p4u commented Jun 26, 2023

fix the check please @altergui

@dennis-tra
Copy link

Great that you're taking the time to check this out!

I'm curious how your experience is. Quick question, are you using the IPFS DHT, another bigger DHT network, or a separate own one?

@altergui altergui force-pushed the f/optimistic-provide branch 2 times, most recently from 6679650 to 24fc66c Compare June 26, 2023 18:49
@p4u
Copy link
Member

p4u commented Jun 26, 2023

Great that you're taking the time to check this out!

I'm curious how your experience is. Quick question, are you using the IPFS DHT, another bigger DHT network, or a separate own one?

We are using IPFS DHT.

@p4u
Copy link
Member

p4u commented Jun 27, 2023

We are not logging any metric regarding the latency on the DHT node discovery.

Before merging this experimental feature, it would be great to add some ipfs-connect metrics to Prometheus @altergui

Feel free to comment and to discard this idea.

@dennis-tra
Copy link

When you get around adding metrics from before and after this change it would be great if you shared them with us. We would be interested in your experience with that feature 👍

@altergui
Copy link
Contributor Author

altergui commented Oct 11, 2023

sorry for taking so long, finally merged the stats in #1090 today
we'll collect current data (without optimistic provide) for a week, then merge this PR to enable optimistic provide, and report any differences

@dennis-tra
Copy link

Great, thanks for the ping! Looking forward to the results 👍

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6482647686

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.007%) to 57.942%

Files with Coverage Reduction New Missed Lines %
vochain/transaction/admin_tx.go 2 50.0%
vochain/transaction/transaction.go 2 41.65%
Totals Coverage Status
Change from base Build 6482114561: -0.007%
Covered Lines: 12917
Relevant Lines: 22293

💛 - Coveralls

@dennis-tra
Copy link

From your linked PR #1090 I saw that you added a new metric with the help text: "The time it takes FindPeers to discover peers".

Just to clarify: Instead of improving the GET performance in the DHT, Optimistic Provide is designed to accelerate the PUT performance so that delay-sensitive applications can benefit from faster content publication.

@altergui altergui self-assigned this Oct 18, 2023
@dennis-tra
Copy link

Hey everyone, just wanted to briefly follow up here and ask about the status

@p4u
Copy link
Member

p4u commented Dec 12, 2023

Now we have some metrics to compare.

We gonna merge this into the development network first, then on stage and finally production.

@p4u p4u merged commit a8b87d0 into main Dec 12, 2023
16 checks passed
@p4u p4u deleted the f/optimistic-provide branch December 12, 2023 12:07
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.

Improved Performance in go-libp2p-kad-dht: New Feature Reduces PUT/Provide Latencies to <1s
4 participants