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. |
| 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
![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 |
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
|
| }); | ||
|
|
||
| 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
|
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.7", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#83682d1ae77705877b81414f39aacce4e27e0ceb", |
There was a problem hiding this comment.
Arsenal is pinned to a commit hash instead of a tag. Per project conventions, git-based deps (arsenal, vaultclient, bucketclient, werelogs, utapi, scubaclient) should 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": "./scality-cloudserverclient-v1.0.9.tgz", |
There was a problem hiding this comment.
@scality/cloudserverclient was moved from devDependencies to dependencies and changed from a registry version to a local .tgz file. Committing binary tarballs to the repo is fragile — it bloats the repo, makes updates opaque, and breaks reproducibility. Additionally, cloudserverclient.tgz is also committed but unused (duplicate of this file). Consider publishing the package to a registry or at least removing the duplicate cloudserverclient.tgz.
— Claude Code
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
32f6a98 to
a444e53
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.7", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#f6b6e2a37675e76186326e22c17b7a4cef77f5e0", |
There was a problem hiding this comment.
Arsenal dependency pinned to a commit hash instead of a tag. Git-based deps should pin to a tag per project conventions.
— Claude Code
| "werelogs": "scality/werelogs#8.2.2", | ||
| "ws": "^8.18.0", | ||
| "@scality/cloudserverclient": "./scality-cloudserverclient-v1.0.9.tgz", | ||
| "xml2js": "^0.6.2" |
There was a problem hiding this comment.
@scality/cloudserverclient is installed from a local tarball checked into the repo. Binary files bloat git history permanently. Consider publishing to a registry or referencing via a git URL with a tag, consistent with how other Scality deps (arsenal, bucketclient, etc.) are managed.
— Claude Code
Review by Claude Code |
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
a444e53 to
eb0241a
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.7", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#f6b6e2a37675e76186326e22c17b7a4cef77f5e0", |
There was a problem hiding this comment.
Arsenal is pinned to a raw commit hash 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": "./scality-cloudserverclient-v1.0.9.tgz", |
There was a problem hiding this comment.
@scality/cloudserverclient moved from devDependencies (registry) to dependencies (local tgz). Shipping a checked-in tarball is fragile — the tgz won't get security patches unless someone manually rebuilds and re-commits it. Also, this is now a production dependency but previously was dev-only, which is a significant change.
— Claude Code
| }], | ||
| }, | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Missing after cleanup hook. Three test buckets are created in before() but never deleted. Add an after() to clean them up so parallel test runs don't leak buckets.
— Claude Code
|
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
eb0241a to
92f01c0
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.7", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#f6b6e2a37675e76186326e22c17b7a4cef77f5e0", |
There was a problem hiding this comment.
Arsenal is pinned to a raw commit hash instead of a semver tag. Project convention requires git-based deps to pin to a tag (e.g. 8.4.7).
— 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.
Both cloudserverclient.tgz and scality-cloudserverclient-v1.0.9.tgz are committed as binary artifacts. Only the latter is referenced in package.json and Dockerfile — cloudserverclient.tgz is an orphan. Binary tarballs inflate the git history permanently; consider publishing to the package registry or removing the unused file.
— Claude Code
| const olderMvId = makeMicroVersionId(); | ||
| const newerMvId = makeMicroVersionId(); | ||
| await putMetadata(key, newerMvId); | ||
| try { |
There was a problem hiding this comment.
This test uses try/catch + assert.fail while the test at line 139 in the same describe block uses assert.rejects. Prefer assert.rejects for consistency.
— Claude Code
| const newerMvId = makeMicroVersionId(); | ||
|
|
||
| await putMetadata(key, olderMvId); | ||
| const output = await putMetadata(key, newerMvId); |
There was a problem hiding this comment.
output is assigned but never used — the test reads from GetMetadataCommand instead.
```suggestion
await putMetadata(key, newerMvId);
<br><br>— Claude Code
Review by Claude Code |
SylvainSenechal
force-pushed
the
improvement/CLDSRV-897
branch
from
92f01c0 to
365213b
Compare
| "@opentelemetry/instrumentation-mongodb": "~0.69.0", | ||
| "@smithy/node-http-handler": "^3.0.0", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#8.4.7", | ||
| "arsenal": "git+https://github.com/scality/Arsenal#f6b6e2a37675e76186326e22c17b7a4cef77f5e0", |
There was a problem hiding this comment.
Arsenal is pinned to a commit hash instead of a tag. Per project conventions, git-based deps must pin to a tag, not a branch or commit.
— Claude Code
| omVal.isDeleteMarker && | ||
| omVal.replicationInfo && | ||
| omVal.replicationInfo.status === 'REPLICA' && | ||
| (omVal.replicationInfo.isReplica || omVal.replicationInfo.status === 'REPLICA') && |
There was a problem hiding this comment.
Inconsistent isReplica check: this line uses a truthy check (isReplica), but lines 654, 671, and 750 use strict equality (isReplica === true). Use strict equality here for consistency.
— Claude Code
| const newerMvId = makeMicroVersionId(); | ||
|
|
||
| await putMetadata(key, olderMvId); | ||
| const output = await putMetadata(key, newerMvId); |
There was a problem hiding this comment.
output is assigned but never used. Remove the assignment.
— 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