fix: replace pre-existing pins for MFS root#7864
Merged
aschmahmann merged 9 commits intopetar/pinmfsfrom Jan 21, 2021
Merged
Conversation
aschmahmann
reviewed
Jan 15, 2021
aschmahmann
reviewed
Jan 15, 2021
| existingRequestID := "" // is there any pre-existing MFS pin with pinName (for any CID)? | ||
| alreadyPinned := false // is CID for current MFS already pinned? | ||
| for ps := range lsPinCh { | ||
| existingRequestID = ps.GetRequestId() |
Contributor
There was a problem hiding this comment.
IIUC this means we're replacing the oldest pin with the correct name. This doesn't seem unreasonable, but it's non-obvious so I wanted to clarify that this is the intended behavior.
Member
Author
There was a problem hiding this comment.
Correct. The assumption is that the name is unique enough to not have more than a single pre-existing one, but if for some reason there is more, we just replace the last one we inspected.
petar
reviewed
Jan 15, 2021
lidel
added a commit
that referenced
this pull request
Jan 16, 2021
This illustrates that logic responsible for finding existing MFS root pin does not account for pins in states other than "pinned".
- find pre-existing pin across all Statuses (not just 'pinned') - replace pre-existing pin or create a new one Without this we would be adding duplicate pins when pinning service is still processing previos pin request (queued, pinning).
If MFS root pin already exists for the CID but has Failed state we Replace it with the same CID to trigger a retry.
iiuc node was sending unnecessary HTTP requests to remote service, this makes sure we skip MFS pin logic if CID did not change
This ensures we use PinStatus.created during the RepinInterval check
Pinning Service may operate in different timezone. Spec suggests use of UTC everywhere, but in case we got different timezone, we normalize to UTC for easier debugging.
This confirms pin gets replaced with new CID when MFS root changes
a26a252 to
1f3f68b
Compare
Member
Author
|
Quick update:
|
Member
Author
|
@petar @aschmahmann Pinata will fix remaining issue on their end (not a blocker for us). |
Contributor
|
merging as @petar gave the 👍. We can continue on the other PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a refactor of #7798 that replaces the existing MFS root pin (if present) instead of creating a new pin.
Without this we would be adding duplicate pins when pinning service
is still processing previous pin request (queued, pinning):
Notes
Replaceat Pinata will still produce duplicate pins, but for a different reason:Replacemethod.Replacefrom rb-pinning-service-api works as expected.