Add CRR Cascaded capabilities#6179
Conversation
Hello sylvainsenechal,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 |
| /** | ||
| * @param {Object} objectMD - plain object metadata (not an ObjectMD instance) | ||
| */ | ||
| function prepareMetadataForCascadedCrr(objectMD) { |
There was a problem hiding this comment.
Could be in arsenal 🤔
There was a problem hiding this comment.
no i think its fine here (talking to myself)
| bucketName: request.bucketName, | ||
| objectKey: request.objectKey, | ||
| }); | ||
| return callback(errors.OperationAborted); |
There was a problem hiding this comment.
many 409 available in arsenal. we can pick this one, maybe some other ones, or create our own
| // Accept both the legacy status='REPLICA' and the new isReplica flag | ||
| // so that cascade hops (which arrive with status='PENDING') are also | ||
| // recognised as replicas. | ||
| const isReplica = omVal.replicationInfo.isReplica === true |
There was a problem hiding this comment.
Should probably use the new helper function from arsenal : getReplicationIsReplica ?
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "@hapi/joi": "^17.1.1", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.2", | ||
| "arsenal": "file:../Arsenal", |
There was a problem hiding this comment.
arsenal and @scality/cloudserverclient are pointing to local filesystem paths (file:../Arsenal, file:../cloudserverclient) instead of pinned git tags. This will break installs for anyone who does not have these directories locally, and CI will fail to resolve them.
| "arsenal": "file:../Arsenal", | |
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.2", |
— Claude Code
| "devDependencies": { | ||
| "@eslint/compat": "^1.2.2", | ||
| "@scality/cloudserverclient": "1.0.7", | ||
| "@scality/cloudserverclient": "file:../cloudserverclient", |
There was a problem hiding this comment.
Same issue: file:../cloudserverclient should be pinned to a published version or git tag.
— Claude Code
| }); | ||
|
|
||
| describe('CRR cascade — putMetadata', () => { | ||
| it('second write with the same microVersionId returns loop-detected', async () => { |
There was a problem hiding this comment.
Test names using it() should start with should per project conventions.
— Claude Code
| }], | ||
| }, | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Missing after() hook to clean up the test buckets (TEST_BUCKET, TEST_BUCKET_CRR, DEST_BUCKET). Without cleanup, leftover buckets accumulate across test runs and can cause name collisions or resource leaks.
— Claude Code
There was a problem hiding this comment.
bucket/keys names are randomized
I intentionnally didnt add any after to keep the test and file lighter.
Do we want to add after cleanup ?
Review by Claude Code |
| if (nextReplInfo && nextReplInfo.backends.length > 0) { | ||
| omVal.replicationInfo = nextReplInfo; | ||
| } else { | ||
| omVal.replicationInfo = { |
There was a problem hiding this comment.
maybe this should be an arsenal function as its updating metadata
There was a problem hiding this comment.
all these worked locally, need to wait for the bumps now
| "@hapi/joi": "^17.1.1", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.2", | ||
| "arsenal": "file:../Arsenal", |
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
c15fb2b to
7912480
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6179 +/- ##
===================================================
+ Coverage 85.25% 85.33% +0.08%
===================================================
Files 208 208
Lines 13919 13963 +44
===================================================
+ Hits 11867 11916 +49
+ Misses 2052 2047 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| }); | ||
|
|
||
| describe('CRR cascade — putMetadata', () => { | ||
| it('second write with the same microVersionId returns loop-detected', async () => { |
There was a problem hiding this comment.
Test names should start with "should" per project conventions (e.g. should return loop-detected on second write with the same microVersionId).
— Claude Code
| "@hapi/joi": "^17.1.1", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.2", | ||
| "arsenal": "file:../Arsenal", |
There was a problem hiding this comment.
arsenal and @scality/cloudserverclient use file: local path references. These must be pinned to a tag or version before merging — file: references will break any install outside of your local workstation.
— Claude Code
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
7912480 to
973261a
Compare
| } | ||
|
|
||
| const encodedMicroVersionId = request.headers['x-scal-micro-version-id']; | ||
| const incomingRaw = encodedMicroVersionId ? decodeMicroVersionId(encodedMicroVersionId) : null; |
There was a problem hiding this comment.
The decode error handling here differs from putMetadata (line 574-575), which normalises the result immediately so incomingRaw is guaranteed non-Error. Here incomingRaw can hold an Error object, then is checked later with !(incomingRaw instanceof Error). Consider aligning with the putMetadata pattern for consistency.
— Claude Code
| "@hapi/joi": "^17.1.1", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.2", | ||
| "arsenal": "file:../Arsenal", |
There was a problem hiding this comment.
arsenal and @scality/cloudserverclient point to local file:../ paths. These must be pinned to git tags before merging — anyone cloning this repo won't have these sibling directories.
— Claude Code
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
973261a to
0dea4be
Compare
| // Old objects without microVersionId: data is already at destination. | ||
| // Cloudserver skips the write and returns null body with no header. | ||
| // Backbeat detects Location===null and treats it as partAlreadyAtDest. | ||
| const { versioning } = require('arsenal'); |
There was a problem hiding this comment.
Move this require('arsenal') to the top of the file alongside the other imports. require() calls should never be inside describe or it() blocks.
— Claude Code
| }], | ||
| }, | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Missing after() hook to delete the three test buckets (TEST_BUCKET, TEST_BUCKET_CRR, DEST_BUCKET) and their objects. This leaves artifacts behind and can cause flaky runs if bucket names collide.
— Claude Code
| "@aws-sdk/client-iam": "^3.930.0", | ||
| "@aws-sdk/client-s3": "^3.1013.0", | ||
| "@aws-sdk/client-sts": "^3.930.0", | ||
| "@aws-sdk/crc64-nvme-crt": "^3.989.0", |
There was a problem hiding this comment.
arsenal uses a file: path pointing to a sibling directory. This will not resolve in CI or for other contributors and violates dependency pinning policy (git-based deps must pin to a tag). Replace with a pinned version before merging, e.g. git+https://github.com/scality/Arsenal#8.4.2
— Claude Code
| "@aws-sdk/crc64-nvme-crt": "^3.989.0", | ||
| "@azure/storage-blob": "^12.28.0", | ||
| "@hapi/joi": "^17.1.1", | ||
| "@smithy/node-http-handler": "^3.0.0", |
There was a problem hiding this comment.
Same issue: file: path must be replaced with a pinned version (tag or npm version) before merging.
— Claude Code
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
464e7f2 to
a0f67b2
Compare
maeldonn
left a comment
maeldonn
left a comment
There was a problem hiding this comment.
I think we can remove the changes from this PR that are already covered in the other one. Once that's done, we can rebase this branch onto the other PR (since Prettier is already included there).
| node-gyp \ | ||
| typescript@4.9.5 | ||
| COPY package.json yarn.lock /usr/src/app/ | ||
| COPY package.json yarn.lock scality-cloudserverclient-v1.0.9.tgz /usr/src/app/ |
There was a problem hiding this comment.
Don't forget to revert, I guess it's for testing
| return callback(errors.OperationAborted); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
You need to use the helper writeContinue() to allow the client to streamData.
3 participants
ISSUE : CLDSRV-897
Crr cascaded design : https://github.com/scality/citadel/pull/349
Related PRs :
Arsenal : scality/Arsenal#2628
CloudserverClient : scality/cloudserverclient#24
Backbeat : scality/backbeat#2747
S3utils : scality/s3utils#395