Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: allow skipping publish of existing advert #117

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Feb 10, 2025

If one or multiple users upload the same blob multiple times the assert/index invocation will create an advert that already exists. This will cause ipni-publisher to return ErrAlreadyAdvertised and the invocation fails. This PR ignores ErrAlreadyAdvertised because, that's fine, whatever.

It also adds a mutex around publish per storacha/ipni-publisher#3

Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/service/providerindex/providerindex.go 88.88% 1 Missing ⚠️
Files with missing lines Coverage Δ
pkg/service/providerindex/providerindex.go 55.60% <88.88%> (+19.75%) ⬆️

... and 1 file with indirect coverage changes

@alanshaw alanshaw requested a review from fforbeck February 10, 2025 16:40
Copy link
Member

@volmedo volmedo left a comment

Choose a reason for hiding this comment

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

left a couple nitpicks, but looks good

@@ -36,6 +38,7 @@ type ProviderIndexService struct {
findClient ipnifind.Finder
publisher publisher.Publisher
legacyClaims LegacyClaimsFinder
mutex *sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

I'd use a plain sync.Mutex here rather than the pointer.

@@ -46,6 +49,7 @@ func New(providerStore types.ProviderStore, findClient ipnifind.Finder, publishe
findClient: findClient,
publisher: publisher,
legacyClaims: legacyClaims,
mutex: &sync.Mutex{},
Copy link
Member

Choose a reason for hiding this comment

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

and skip its initialization as the default value already provides a usable mutex

alanshaw and others added 2 commits February 11, 2025 09:35
@alanshaw alanshaw requested a review from volmedo February 11, 2025 09:37
@alanshaw alanshaw merged commit 9b1c5d6 into main Feb 11, 2025
9 checks passed
@alanshaw alanshaw deleted the fix/allow-skip-publish-existing-advert branch February 11, 2025 09:50
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants