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(pinmfs): log error if pre-existing pin failed #8056

Merged
merged 2 commits into from May 17, 2021

Conversation

BogdanStirbat
Copy link
Contributor

@BogdanStirbat BogdanStirbat commented Apr 6, 2021

Add an error log message to remotepinning/mfs, in case a pin request failed.

Note from @lidel: this gets us closer to addressing #7917 by adding error logging on node start, but proper fix requires a larger refactor in a separate PR.

@Stebalien Stebalien requested a review from lidel April 15, 2021 04:33
@ipfs ipfs deleted a comment from welcome bot Apr 23, 2021
This cleans up debug logging and ensures we use %q for data read from
remote services. We log pre-existing pins for MFS root and print ERROR
if pin for current root has status "failed", otherwise, we log to debug.
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, @BogdanStirbat, LGTM (tweaked messages a bit, so things look more uniform now)

@Stebalien / @aschmahmann this should be ready for final review.

FYSA this PR does not change any functionality, only adds/tweaks logs produced on ipfs log level remotepinning/mfs on error and debug levels

Comment on lines +220 to +224
if ps.GetPin().GetCid() == cid && ps.GetStatus() == pinclient.StatusFailed {
mfslog.Errorf(pinStatusMsg, svcName, pinclient.StatusFailed, cid, existingRequestID)
} else {
mfslog.Debugf(pinStatusMsg, svcName, ps.GetStatus(), ps.GetPin().GetCid(), existingRequestID)
}
Copy link
Member

@lidel lidel Apr 23, 2021

Choose a reason for hiding this comment

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

This is the only real change in this PR, everything else is just my opportunistic cleanup of already existing debug logs :)

👉 error log level is used only when failure is for the current MFS root CID, everything else is logged to debug:

2021-04-23--23-24-06

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a bit awkward since if your node is offline and you start mutating MFS your log will fill up with errors?

Copy link
Member

@lidel lidel May 17, 2021

Choose a reason for hiding this comment

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

No, pinning is lazy atm, so service will return queued status and then try to pin it for some time (eg. Pinata won't return failed until N hours passed without any new bytes found).

In practice, the ERROR added in this PR is only printed once, when you restart node.
Main value here is improved logs when ipfs log level remotepinning/mfs debug is set.

@lidel lidel changed the title pinmfs: log error if pin failed refactor(pinmfs): log error if pre-existing pin failed Apr 23, 2021
@BigLep BigLep added kind/enhancement A net-new feature or improvement to an existing feature P3 Low: Not priority right now labels May 17, 2021
@lidel lidel merged commit 075cc8f into ipfs:master May 17, 2021
aschmahmann pushed a commit that referenced this pull request May 27, 2021
* pinmfs: log error if pin failed
* refactor: log pre-existing remote mfs pins

This cleans up debug logging and ensures we use %q for data read from
remote services. We log pre-existing pins for MFS root and print ERROR
if pin for current root has status "failed", otherwise, we log to debug.

Co-authored-by: Marcin Rataj <lidel@lidel.org>
(cherry picked from commit 075cc8f)
aschmahmann pushed a commit that referenced this pull request May 27, 2021
* pinmfs: log error if pin failed
* refactor: log pre-existing remote mfs pins

This cleans up debug logging and ensures we use %q for data read from
remote services. We log pre-existing pins for MFS root and print ERROR
if pin for current root has status "failed", otherwise, we log to debug.

Co-authored-by: Marcin Rataj <lidel@lidel.org>
(cherry picked from commit 075cc8f)
@aschmahmann aschmahmann mentioned this pull request May 28, 2021
71 tasks
@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature P3 Low: Not priority right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants