Adopt timestamp-ordered microVersionId in CRR resync tool#395
Conversation
Hello maeldonn,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 |
1808039 to
1d860c7
Compare
Review by Claude Code |
Review by Claude Code |
SylvainSenechal
left a comment
There was a problem hiding this comment.
reviewed but let's wait a bit before merging, see how we end up doing things in arsenal
1d860c7 to
19a46e6
Compare
| "@smithy/util-retry": "^4.0.7", | ||
| "JSONStream": "^1.3.5", | ||
| "arsenal": "git+https://github.com/scality/arsenal#8.2.36", | ||
| "arsenal": "git+https://github.com/scality/arsenal#2c429ab35a5ac82c3dafa5a0296a49a23a9c8a4a", |
There was a problem hiding this comment.
arsenal is pinned to a bare commit hash instead of a tag. No tag exists yet for 2c429ab. Git-based deps in this project should pin to a tag for readability and traceability. Please publish a tag on Arsenal for this commit and reference it here.
— Claude Code
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## development/1 #395 +/- ##
=================================================
+ Coverage 45.44% 45.47% +0.02%
=================================================
Files 88 88
Lines 6526 6529 +3
Branches 1372 1372
=================================================
+ Hits 2966 2969 +3
Misses 3510 3510
Partials 50 50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SylvainSenechal
left a comment
There was a problem hiding this comment.
Ok for me, let's wait a minute :
- Arsenal should be the first merge (and need to update package json here)
- Need to decide about branch, probably 1.18 insteaf od 1.17
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
19a46e6 to
b489f99
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
423ddb9 to
714b486
Compare
96ab7cc to
1809e7a
Compare
|
LGTM |
Bump arsenal to pick up the new microVersionId format (ts + seq + repGroupId), required by cascaded CRR for loop detection and stale event handling. The resync tool bypasses CloudServer's S3 API, so it generates a random per-instance repGroupId and passes it to updateMicroVersionId() to avoid colliding with concurrent writers. Issue: S3UTILS-234
42ba3d2 to
128b8c7
Compare
| // collisions with concurrent writers. Sizes match LENGTH_RG and | ||
| // LENGTH_ID in arsenal's VersionID module. | ||
| this.replicationGroupId = crypto.randomBytes(4).toString('hex').slice(0, 7); | ||
| this.instanceId = crypto.randomBytes(3).toString('hex').slice(0, 6); |
There was a problem hiding this comment.
No unit test verifies the format of the generated replicationGroupId (7 hex chars) or instanceId (6 hex chars), or that updateMicroVersionId receives them. Since these sizes must match arsenal's LENGTH_RG and LENGTH_ID constants for cascaded CRR loop detection to work, a simple test asserting the lengths and hex-only content would catch regressions if the generation logic is ever refactored.
Bump arsenal to pick up the new microVersionId format (ts + seq + repGroupId), required by cascaded CRR for loop detection and stale event handling. The resync tool bypasses CloudServer's S3 API, so it generates a random per-instance repGroupId and passes it to updateMicroVersionId() to avoid colliding with concurrent writers.
Issue: S3UTILS-234
Related PRs :
Arsenal : scality/Arsenal#2628
Cloudserver : scality/cloudserver#6179
CloudserverClient : scality/cloudserverclient#24
Backbeat : scality/backbeat#2747