CLDSRV-960: backbeat checksum cleanup - #6241
Conversation
Hello leif-scality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6241 +/- ##
===================================================
- Coverage 86.23% 86.17% -0.06%
===================================================
Files 212 212
Lines 14536 14519 -17
===================================================
- Hits 12535 12512 -23
- Misses 2001 2007 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
518ba53 to
67df9dd
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| }); | ||
| return callback(err); | ||
| } | ||
| if (contentMD5 !== md5) { |
There was a problem hiding this comment.
Unrelated to this PR, but I noticed it.
In this case, we return BadDigest, but the storeObject has already succeeded.
Doesn't that leave an orphan?
(same in putObject)
There was a problem hiding this comment.
Created this ticket https://scality.atlassian.net/browse/CLDSRV-967
| * @return {{ error: Arsenal.Error|null, stream: stream.Readable|null, | ||
| * primaryChecksumStream: ChecksumTransform|null, | ||
| * secondaryChecksumStream: ChecksumTransform|null, | ||
| * contentSHA256Stream: ContentSHA256Transform|null }} |
There was a problem hiding this comment.
In some cases we return undefined instead of null for contentSHA256Stream.
67df9dd to
435411f
Compare
| ); | ||
| secondaryChecksumStream.on('error', onStreamError); | ||
| stream = v4Transform.pipe(secondaryChecksumStream); | ||
| const secondaryChecksumStream = createChecksumStream(secondary, onStreamError, log); |
There was a problem hiding this comment.
It's clearer now than it used to be which is cool, but we seem to have a repeated pattern to create primary and/or secondary checksums and return them (without the trailer), maybe worth another helper that complements createChecksumStream to do that?
| * digest/algoName properties, or null if no checksum was computed | ||
| * @param {object} log - request logger instance | ||
| * @param {function} cb - callback to send error or move to next task | ||
| * @return {function} - calls callback with arguments: |
There was a problem hiding this comment.
Seems we are missing the checksum argument description in the return value
| const result = prepareStream(request, null, null, log, () => {}); | ||
| assert.strictEqual(result.error, null); | ||
| assert.strictEqual(result.stream, request); | ||
| assert.strictEqual(result.primaryChecksumStream, null); |
There was a problem hiding this comment.
Nit: this test doesn't check the secondary and SHA256 streams nullity
435411f to
153e2fb
Compare
|
ping |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue CLDSRV-960. Goodbye leif-scality. The following options are set: approve |
4 participants
x-amz-checksum-headers in backbeat routes