-
Notifications
You must be signed in to change notification settings - Fork 31
Fix checksum logic in pelican object stat
#2847
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
Fix checksum logic in pelican object stat
#2847
Conversation
turetske
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some clarifying questions, just so I understand the fix.
| delete(remaining, info.Algorithm) | ||
| collected = true | ||
| } | ||
| if len(remaining) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify more my own understanding, since you only delete info.Algorithm from remaining when there is no checksumErr, this is only true if all the checksums have evaluated with no error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. The delete(remaining, info.Algorithm) happens whenever fetchChecksum successfully returns checksums from any URL in statUrls. So len(remaining) == 0 indicates "all requested checksums were successfully collected from at least one server each" - not necessarily that there were zero errors. You could have errors on some URLs but still succeed if another URL provides all the checksums.
| if checksumErr == nil { | ||
| checksumErr = errors.New("no checksum data returned by server") | ||
| } | ||
| err = errors.Wrap(checksumErr, "failed to fetch checksums") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this mean if the server doesn't have checksumming, this will throw an error? Or is there a check higher up in the algorithm I can't see due to the nature of the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but only if the caller explicitly requested checksums. This is intentional behavior, if a caller explicitly requests checksums and the server can't provide them, that's a failure condition. If no checksums are requested, this code path is skipped entirely and everything proceeds normally.
turetske
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #2827. When running
pelican object stat --checksums sha, the client would display what checksum the server returned without any indication that the requested checksum type wasn't provided. Now we track the requested checksums and track that they are satisfied. We now log a warning when the requested checksum types are not returned by the server.