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. |
| /** | ||
| * @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
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
| }); | ||
|
|
||
| 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
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).
There was a problem hiding this comment.
yeah for testing i shipped cloudserverclient with it
| 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.
There was a problem hiding this comment.
ok let me see I thought this was somewhat automatic
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
2 times, most recently
from
3969d0e to
daef2e1
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.5", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#2c429ab35a5ac82c3dafa5a0296a49a23a9c8a4a", |
There was a problem hiding this comment.
Arsenal is pinned to a raw commit hash instead of a version tag. Per project conventions, git-based dependencies (arsenal, vaultclient, bucketclient, werelogs, utapi, scubaclient) should pin to a tag.
— Claude Code
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
daef2e1 to
88edec5
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.5", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#2c429ab35a5ac82c3dafa5a0296a49a23a9c8a4a", |
There was a problem hiding this comment.
arsenal is pinned to a commit hash (2c429ab) instead of a tag. Per project conventions, git-based deps must pin to a tag, not a branch or commit.
— Claude Code
| "vaultclient": "scality/vaultclient#8.5.3", | ||
| "werelogs": "scality/werelogs#8.2.2", | ||
| "ws": "^8.18.0", | ||
| "@scality/cloudserverclient": "/Users/sylvainsenechal/Desktop/cloudserverclient/cloudserverclient.tgz", |
There was a problem hiding this comment.
@scality/cloudserverclient points to a local filesystem path (/Users/sylvainsenechal/Desktop/...). This will break for any other developer and in CI. It should be a registry version or a git-based dependency with a tag.
Also, this was moved from devDependencies to dependencies — since it is now imported in production code (lib/routes/routeBackbeat.js), confirm this is intentional and the package should indeed be a production dependency.
— Claude Code
| 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.
Binary .tgz file checked into the repository. This should not be committed — use a proper npm registry reference or git-based dependency instead.
— Claude Code
| fast-xml-parser "^5.5.7" | ||
|
|
||
| "@scality/cloudserverclient@file:../cloudserverclient/cloudserverclient.tgz": | ||
| version "1.0.9" |
There was a problem hiding this comment.
The yarn.lock contains a resolved path to a local filesystem (/Users/sylvainsenechal/Desktop/...) which will not resolve for other developers or CI. This needs to be updated to use a proper registry or git reference.
— Claude Code
| const obj = Object.assign({ | ||
| 'content-length': Buffer.byteLength(OBJECT_BODY), | ||
| 'content-type': 'text/plain', | ||
| 'last-modified': new Date().toISOString(), |
There was a problem hiding this comment.
Test names using it() should start with "should" per project conventions. For example: "should return loop-detected on second write with the same microVersionId".
This applies to all it() tests in this file.
— Claude Code
| @@ -174,6 +175,47 @@ describe('routeBackbeat', () => { | |||
| assert.deepStrictEqual(mockResponse.body, [{}]); | |||
There was a problem hiding this comment.
Test name should start with "should" per project conventions.
— Claude Code
| return _respondWithHeaders(response, | ||
| { code: VersionIdCollisionException.name, message: 'version id already at destination' }, | ||
| { 'x-scal-micro-version-id': objMd.microVersionId ? encode(objMd.microVersionId) : '' }, | ||
| log, callback, 409); |
There was a problem hiding this comment.
putData returns a 409 response before calling writeContinue. If the client sent Expect: 100-continue with a body, returning 409 before consuming (or explicitly rejecting) the request stream may leave the connection in a broken state — the client could be waiting for 100-continue and then start sending data that nobody reads. Consider either calling request.destroy() or request.resume() before responding, or calling writeContinue and consuming the body before checking.
— Claude Code
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
88edec5 to
a90d5b2
Compare
| "@azure/storage-blob": "^12.28.0", | ||
| "@hapi/joi": "^17.1.1", | ||
| "@opentelemetry/api": "^1.9.0", | ||
| "@opentelemetry/instrumentation-http": "~0.218.0", |
There was a problem hiding this comment.
Arsenal is pinned to a commit hash instead of a tag. Per project conventions, git-based dependencies (arsenal, vaultclient, bucketclient, werelogs, utapi, scubaclient) must pin to a tag, not a branch or commit hash.suggestion<br> "arsenal": "git+https://github.com/scality/Arsenal#8.4.5",<br>
— Claude Code
| "vaultclient": "scality/vaultclient#8.5.3", | ||
| "werelogs": "scality/werelogs#8.2.2", | ||
| "ws": "^8.18.0", | ||
| "@scality/cloudserverclient": "./scality-cloudserverclient-v1.0.9.tgz", |
There was a problem hiding this comment.
@scality/cloudserverclient is vendored as a binary tarball (scality-cloudserverclient-v1.0.9.tgz) committed to the repo. Binary artifacts should be published to a package registry and referenced by version. This inflates the repo size and makes auditing the dependency harder.
— Claude Code
Review by Claude Code |
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