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: add reprovide #2785

Merged
merged 13 commits into from
Oct 28, 2024
Merged

feat: add reprovide #2785

merged 13 commits into from
Oct 28, 2024

Conversation

achingbrain
Copy link
Member

Adds a reprovide queue to kad-dht to republish provider records two hours before they would expire.

Adds a function to the content routing interface to cancel the reprovide.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

Adds a reprovide queue to kad-dht to republish provider records two
hours before they would expire.

Adds a function to the content routing interface to cancel the
reprovide.
@achingbrain achingbrain requested a review from a team as a code owner October 24, 2024 11:59
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

a few comments

packages/kad-dht/src/providers.ts Outdated Show resolved Hide resolved
this.log('nothing to delete')
}

this.log('Cleanup successful (%dms)', Date.now() - start)
Copy link
Member

Choose a reason for hiding this comment

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

should we be emitting metrics for this? I know kubo has had a lot of issues with reprovide time, we may want to get ahead of any potential issues with some metrics.

Copy link
Member Author

Choose a reason for hiding this comment

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

The actual re-providing is done in a queue which has metrics for it's length, in-flight tasks, etc. I think this should be good enough to start with, we can always add more in future?

packages/kad-dht/test/providers.spec.ts Outdated Show resolved Hide resolved
packages/kad-dht/test/providers.spec.ts Outdated Show resolved Hide resolved
packages/kad-dht/test/rpc/handlers/get-value.spec.ts Outdated Show resolved Hide resolved
packages/kad-dht/test/rpc/handlers/put-value.spec.ts Outdated Show resolved Hide resolved
packages/kad-dht/test/utils/create-peer-id.ts Outdated Show resolved Hide resolved
packages/kad-dht/test/utils/create-peer-id.ts Show resolved Hide resolved
@achingbrain achingbrain merged commit 52b3b1a into main Oct 28, 2024
30 checks passed
@achingbrain achingbrain deleted the feat/add-reprovide branch October 28, 2024 08:08
@achingbrain achingbrain mentioned this pull request Oct 28, 2024
@@ -15,15 +15,21 @@ export const MAX_RECORD_AGE = 36 * hour

export const PROTOCOL = '/ipfs/kad/1.0.0'

export const RECORD_KEY_PREFIX = '/dht/record'
export const PROVIDERS_VALIDITY = 24 * hour
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It would seem so, can haz PR?

this.log.trace('re-provided %c', cid)
}, {
signal: this.shutdownController?.signal,
cid
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you pass in the cid if you have it via the closure?

// reprovide
await this.reprovide(options.cid, options)
} finally {
this.reprovideTimeout.cleanUp(signal)
Copy link
Contributor

Choose a reason for hiding this comment

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

This runs irrespective of whether the reprovide for the CID succeeds. Is that a problem?

achingbrain added a commit that referenced this pull request Nov 16, 2024
Increase provider record validity duration based on spec as discussed in #2785

---------

Co-authored-by: Daniel N <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
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