Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

feat: add namesys publish options #94

Merged
merged 3 commits into from
Jan 24, 2023
Merged

feat: add namesys publish options #94

merged 3 commits into from
Jan 24, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Dec 7, 2022

options/namesys/opts.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the feat/publish-options branch 2 times, most recently from 0d8e929 to 77d51fc Compare December 13, 2022 10:29
func DefaultPublishOptions() PublishOptions {
return PublishOptions{
EOL: time.Now().Add(defaultRecordEOL),
TTL: time.Minute,
Copy link
Member

Choose a reason for hiding this comment

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

This default should be moved to a const too (DefaultIPNSRecordTTL).

💭 I wonder if 1 minute is a good default. We are caching IPNS lookup results for 1m in Kubo, so I understand setting TTL to 1m keeps the current behavior, but that is just bare minimum for performance reasons.

For context, when you add DNS record at many registrars, the default TTL for DNS records is either 12 hours or 1 hour, and is lowered manually by the user to 1 minute only for websites which need fast updates.

If we keep this low from the start, the performance improvement of TTL will be close to zero, because nobody will adjust it to a bigger value (tyranny of the default).

@hacdias I don't want to block this PR on the decision, let's go with 1m for now, but mind opening a PR against https://github.com/ipfs/specs/blob/main/ipns/IPNS.md#ipns-record that adds suggested default to be 12h, so we can then gather feedback?

Copy link
Member Author

@hacdias hacdias Jan 24, 2023

Choose a reason for hiding this comment

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

Will add that to my ToDos (today or tomorrow). Constant exported.

Copy link
Member Author

Choose a reason for hiding this comment

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

options/namesys/opts.go Outdated Show resolved Hide resolved
@hacdias hacdias requested a review from lidel January 24, 2023 13:15
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thank you, this makes things less noisy by keeping all opt consts in the same place.

@lidel lidel merged commit 468dea4 into master Jan 24, 2023
@lidel lidel deleted the feat/publish-options branch January 24, 2023 22:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants