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.

Sign up for GitHub

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

Jump to bottom

feat: increase default Reprovider.Interval #9326

Merged
merged 2 commits into from
Dec 8, 2022
Merged

feat: increase default Reprovider.Interval #9326

merged 2 commits into from
Dec 8, 2022
+40 −33

Conversation

cortze
Copy link
Contributor

@cortze cortze commented Oct 5, 2022 •
edited
Loading

Description

This PR aims to increase kubo's default republish interval from 12h to 22h based on RFM17 as discussed in libp2p/specs#451.

Motivation

The measurements from RFM17 were surprising and promising for the steady provider record's liveness gathered in the IPFS live network. Based on the average of 70% of online PR holders and 70% of PR holders being among the 20 closest peers for over +48 hours, we suggest increasing the republish interval for the CIDs and extending the expiration time to help reduce part of the overhead that providing large sets of CIDs implies.

Why 22 hours?
Since the current expiration time for the PR is set to 24h, setting it to 22h will give enough margin to providers to publish the PR again before they expire.

Note: this PR will be twined with one at go-libp2p-kad-dht to extend the expiration time

Modifications

  • Increase Reprovider.Interval from 12h -> 22h

@welcome

This comment was marked as resolved.

Sign in to view
@yiannisbot yiannisbot mentioned this pull request Oct 5, 2022
Merged
@cortze cortze mentioned this pull request Oct 5, 2022
Merged
@cortze
Copy link
Contributor Author

cortze commented Oct 5, 2022

Linking here the PR libp2p/go-libp2p-kad-dht#793 to increase the expiration time of the PRs to 48h as it is directly linked to this PR.

@cortze cortze mentioned this pull request Oct 12, 2022
Merged
@guseggert guseggert added this to the Best Effort Track milestone Oct 13, 2022
@guseggert guseggert added the P2 Medium: Good to have, but can wait until someone steps up label Oct 13, 2022
@guseggert
Copy link
Contributor

Some feedback from our triage discussion:

  • This should be changed to an optional string
  • We need a config migration here that changes existing config values from 12h -> 22h, otherwise this would only take effect for new repos

@yiannisbot yiannisbot mentioned this pull request Nov 8, 2022
Closed
6 tasks
lidel
lidel requested changes Nov 17, 2022
View reviewed changes
config/init.go Outdated
Comment on lines 79 to 80
Interval: "22h",
Strategy: "all",
Copy link
Member

@lidel lidel Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we want this change to be effective and applied to older nodes, this needs both a refactor and a config migration.

  1. Both fields defined in

    kubo/config/reprovider.go

    Lines 4 to 5 in 83b3ec4

    Interval string // Time period to reprovide locally stored objects to the network
    Strategy string // Which keys to announce

    should be replaced with optionalString variants, and the implicit default should no longer be hardcoded in user config – see prior art in refactor(config): remove Swarm.ConnMgr defaults #8913
  2. Config migration should be added, and it should migrate users running with default values to implicit defaults
    • If Interval = 12h and Strategy = all, then remove both from config (switch to implicit default added in step 1)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good callout @lidel .

Is this approachable for a non-maintainer to do? @cortze can you take this?

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 could take a look at it if that is okey for you, will come back if there are any questions :)

Copy link
Member

Choose a reason for hiding this comment

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

iiuc @Jorropo will be writing config migration for enabling webtransport by default (#9292), so ok to wait until that example is present, and follow similar path.

This was referenced Nov 17, 2022
Closed
Closed
@yiannisbot yiannisbot mentioned this pull request Nov 25, 2022
Closed
@lidel lidel self-assigned this Dec 7, 2022
@lidel lidel changed the title increase default republish interval feat: increase default republish interval Dec 7, 2022
@lidel lidel force-pushed the cortze/upgrade-reprovider-interval branch 2 times, most recently from 923630e to e929378 Compare December 7, 2022 20:00
@lidel
Copy link
Member

lidel commented Dec 7, 2022 •
edited
Loading

Switched fields to optional, which simplified some code. Let's see if CI passes.

Remaining work here (@lidel)

  • release notes
  • CI green
  • write migration that detects old defaults and sets Reprovider to {} (switching user to new implicit defaults).

lidel
lidel approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is ready for 0.18 #9417 – migrations will continue without blocking this PR (draft in ipfs/fs-repo-migrations#161)

lidel added a commit to ipfs/fs-repo-migrations that referenced this pull request Dec 7, 2022
@lidel
wip: kubo 0.18 config migrations
2788ee3 
ipfs/kubo#9475
ipfs/kubo#9467
ipfs/kubo#9326
@lidel lidel mentioned this pull request Dec 8, 2022
Merged
9 tasks
@lidel
refactor(config): switch to implicit default
05f56a1 
This allows us to adjust default in new release if user did not
specify their own value.
@lidel lidel force-pushed the cortze/upgrade-reprovider-interval branch from e929378 to 05f56a1 Compare December 8, 2022 16:42
@lidel lidel merged commit 72bad5c into ipfs:master Dec 8, 2022
@lidel lidel changed the title feat: increase default republish interval feat: increase default Reprovider.Interval Dec 8, 2022
lidel added a commit to ipfs/fs-repo-migrations that referenced this pull request Dec 9, 2022
@lidel
wip: kubo 0.18 config migrations
0660202 
ipfs/kubo#9475
ipfs/kubo#9467
ipfs/kubo#9326
@lidel lidel mentioned this pull request Dec 10, 2022
Closed
@lidel lidel mentioned this pull request Mar 27, 2023
Merged
@guillaumemichel guillaumemichel mentioned this pull request May 24, 2023
Merged
lidel added a commit that referenced this pull request May 29, 2023
@lidel
docs: Reprovider config
d0123f8 
This updates docs to match new values from #9326
@lidel lidel mentioned this pull request May 29, 2023
Merged
lidel added a commit that referenced this pull request May 29, 2023
@lidel
docs: Reprovider config (#9900)
38556b8 
This updates docs to match new values from #9326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BigLep BigLep BigLep left review comments

@lidel lidel lidel approved these changes

Assignees

@lidel lidel

Labels
P2 Medium: Good to have, but can wait until someone steps up
Projects
No open projects
IPFS Shipyard Team
Archived in project
Milestone
Best Effort Track
Development

Successfully merging this pull request may close these issues.
4 participants
@cortze @guseggert @lidel @BigLep