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

provider: simplify reprovide #890

Merged
merged 10 commits into from
Mar 25, 2025
Merged

provider: simplify reprovide #890

merged 10 commits into from
Mar 25, 2025

Conversation

guillaumemichel
Copy link
Contributor

@guillaumemichel guillaumemichel commented Mar 21, 2025

Follow up to #871 and part of https://github.com/ipshipyard/roadmaps/issues/6 / https://github.com/ipshipyard/roadmaps/issues/7

Hopefully it will help with ipfs/kubo#10714

Previous behavior was hanging on a select waiting for context to be cancelled.

If the warning message 🔔🔔🔔 YOU ARE FALLING BEHIND DHT REPROVIDES! 🔔🔔🔔 keeps coming, while the reprovideInterval is set to 0, it means that Reprovide was called (probably from kubo, with ipfs routing reprovide?).

EDIT: the reprovide warning issue seems to come from kubo

@guillaumemichel guillaumemichel requested a review from a team as a code owner March 21, 2025 14:28
@guillaumemichel guillaumemichel force-pushed the reprovide-scheduling-fix branch 2 times, most recently from e7bea46 to ecd5683 Compare March 21, 2025 17:01
@guillaumemichel guillaumemichel force-pushed the reprovide-scheduling-fix branch from ecd5683 to 46fab87 Compare March 21, 2025 17:14
Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

The stopAndEmptyTimer is not needed since go.123 is required. Call the timer's Stop method instead. Handled in change 0301460

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

As noted by @aschmahmann : If your node reboots do you re-advertise all your data again even if it's within the reprovide interval?

If this is an intended behavior change then it should be documetned in release notes.

@guillaumemichel
Copy link
Contributor Author

If your node reboots do you re-advertise all your data again even if it's within the reprovide interval?

Right, I missed why the reprovide times were persisted to the datastore. I reverted and documented this behavior.

I also corrected a corner case. Previously, a node rebooting, that would have have to reprovide in 1h if it hadn't rebooted, would not reprovide on reboot, but would wait reprovideInterval(=22h) before reproviding, implying that the reproviding would be 21h late.

In the fix, the first provide is either reprovideInterval after the last reprovide written in the datastore, or after initialReprovideDelay if the last reprovide was more than reprovideInterval ago or if no last reprovide date was found.

Added a changelog entry, since this last change actually modifies the behavior.

Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

LGTM

@gammazero gammazero merged commit f6362ad into main Mar 25, 2025
13 checks passed
@gammazero gammazero deleted the reprovide-scheduling-fix branch March 25, 2025 22:56
@lidel lidel mentioned this pull request Mar 26, 2025
27 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.

2 participants