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

refactor: simplify logic for MFS remote pinning #10506

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

gammazero
Copy link
Contributor

@gammazero gammazero commented Sep 5, 2024

MFS pinning logic was returning errors on a channel, only to be logged by a separate goroutine. The errors were not used for any other purpose, so the logic could be significantly simplified by logging the error where it happened instead of returning it on a channel to be logged. This PR removes the unnecessary channel and logging goroutine.

A couple other minor simplifications and consistency changes were also made, such as passing a context as the first argument.

MFS pinning logis was returning errors on a channel, only to be logged by a separate goroutine. The errors were not used for any other purpose, so the logic could be significantly simplified by logging the error where it happened instead of returning it on a channel to be logged. This PR removes the unnecessary channel and logging goroutine.

Some other minor simplifications and consistency changes were also made, such as passing a context as the first argument.
@gammazero gammazero added the skip/changelog This change does NOT require a changelog entry label Sep 5, 2024
@gammazero gammazero requested a review from a team as a code owner September 5, 2024 00:33
@lidel lidel changed the title Simplify logic for MFS pinning refactor: simplify logic for MFS remote pinning Sep 17, 2024
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.

lgtm, thank you for cleaning this up @gammazero

(smoke-tested end-to-end with Pinata, MFS pinning works as expected when enabled)

@lidel lidel merged commit ca4f486 into master Sep 27, 2024
14 checks passed
@lidel lidel deleted the chore/simplify-mfs-pinning branch September 27, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does NOT require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants