Skip to content

Upgrade microVersionId to timestamp-ordered format and add isReplica#2628

Open
maeldonn wants to merge 2 commits into
development/8.4from
improvement/ARSN-578/micro-version-id
Open

Upgrade microVersionId to timestamp-ordered format and add isReplica#2628
maeldonn wants to merge 2 commits into
development/8.4from
improvement/ARSN-578/micro-version-id

Conversation

@maeldonn
Copy link
Copy Markdown
Contributor

@maeldonn maeldonn commented May 22, 2026

Replace the random 8-byte hex microVersionId with a 20-character timestamp-ordered identifier matching the versionId scheme, and add encode/decode/compare helpers in a new MicroVersionID module. Add an optional isReplica flag to ReplicationInfo to distinguish replica writes from user writes, enabling cascaded CRR.

Issue: ARSN-578

Crr cascaded design : https://github.com/scality/citadel/pull/349
Related PRs :
Cloudserver : scality/cloudserver#6179
CloudserverClient : scality/cloudserverclient#24
Backbeat : scality/backbeat#2747
S3utils : scality/s3utils#395

@maeldonn maeldonn requested review from a team, SylvainSenechal and benzekrimaha May 22, 2026 14:11
@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented May 22, 2026

Hello maeldonn,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options
name description privileged authored
/after_pull_request Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval Bypass the pull request author's approval
/bypass_build_status Bypass the build and test status
/bypass_commit_size Bypass the check on the size of the changeset TBA
/bypass_incompatible_branch Bypass the check on the source branch prefix
/bypass_jira_check Bypass the Jira issue check
/bypass_peer_approval Bypass the pull request peers' approval
/bypass_leader_approval Bypass the pull request leaders' approval
/approve Instruct Bert-E that the author has approved the pull request. ✍️
/create_pull_requests Allow the creation of integration pull requests.
/create_integration_branches Allow the creation of integration branches.
/no_octopus Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity Change review acceptance criteria from one reviewer at least to all reviewers
/wait Instruct Bert-E not to run until further notice.
Available commands
name description privileged
/help Print Bert-E's manual in the pull request.
/status Print Bert-E's current status in the pull request TBA
/clear Remove all comments from Bert-E from the history TBA
/retry Re-start a fresh build TBA
/build Re-start a fresh build TBA
/force_reset Delete integration branches & pull requests, and restart merge process from the beginning.
/reset Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented May 22, 2026

Incorrect fix version

The Fix Version/s in issue ARSN-578 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 8.4.3

Please check the Fix Version/s of ARSN-578, or the target
branch of this pull request.

@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

LGTM

Review by Claude Code

Comment thread lib/models/ObjectMD.ts
Comment thread lib/models/ObjectMD.ts
Comment thread lib/models/ObjectMD.ts Outdated
Comment thread lib/versioning/MicroVersionId.ts Outdated
@maeldonn maeldonn force-pushed the improvement/ARSN-578/micro-version-id branch from caf9a51 to 868cae8 Compare May 22, 2026 14:55
Comment thread lib/versioning/MicroVersionId.ts Outdated
Comment thread lib/models/ObjectMD.ts Outdated
Comment thread lib/models/ObjectMD.ts Outdated
@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

  • Breaking API change: updateMicroVersionId() now requires a replicationGroupId argument, which will break all existing callers in downstream repos (CloudServer, Backbeat, etc.). Coordinate updates or consider an optional parameter with a fallback.
    • Make the parameter optional with a random default, or ensure all consumers are updated in the same release cycle.
  • updateMicroVersionId JSDoc says @return itself but the method body has no return this statement.
    • Add return this; for consistency with other ObjectMD setters.

Review by Claude Code

Comment thread lib/versioning/MicroVersionId.ts Outdated
@maeldonn maeldonn force-pushed the improvement/ARSN-578/micro-version-id branch from 868cae8 to 2eae0c5 Compare May 22, 2026 15:22
@maeldonn maeldonn marked this pull request as ready for review May 22, 2026 15:24
@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

LGTM

The changes are well-structured: the new MicroVersionId module correctly mirrors the VersionId timestamp-ordering scheme with its own isolated sequencing state, the isReplica flag is cleanly additive and backward compatible, and the test coverage is thorough (uniqueness, ordering, encode/decode round-trip, compare, padding/truncation).

Review by Claude Code

Comment thread lib/versioning/MicroVersionId.ts Outdated
Comment thread lib/versioning/MicroVersionId.ts Outdated
@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented May 22, 2026

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • 2 peers

@maeldonn maeldonn requested a review from SylvainSenechal May 22, 2026 18:03
Comment thread lib/models/ObjectMD.ts
@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

  • updateMicroVersionId JSDoc says @return itself but the method doesn't return this, breaking the chaining convention used by all other setters in ObjectMD. Since the method is being rewritten, add return this;.

Review by Claude Code

@maeldonn maeldonn force-pushed the improvement/ARSN-578/micro-version-id branch from fbaf3fe to 3160d6b Compare May 22, 2026 18:25
Comment thread lib/models/ObjectMD.ts Outdated
@claude
Copy link
Copy Markdown

claude Bot commented May 22, 2026

LGTM — one breaking change to flag:

- updateMicroVersionId() signature changed to require a replicationGroupId parameter — downstream callers (CloudServer, Backbeat) must be updated in lockstep

The new MicroVersionId module, isReplica field, and timestamp-ordered format all look correct. Backward compatibility for existing stored metadata (old 16-char random hex microVersionIds) is preserved since getMicroVersionId() returns the raw value and _updateFromParsedJSON copies it as-is.

Review by Claude Code

@maeldonn maeldonn force-pushed the improvement/ARSN-578/micro-version-id branch from 3160d6b to 5f4839a Compare May 22, 2026 18:31
Comment thread lib/models/ObjectMD.ts
@SylvainSenechal SylvainSenechal force-pushed the improvement/ARSN-578/micro-version-id branch from 7be6414 to 2d68077 Compare May 29, 2026 13:41
Comment thread lib/models/ObjectMD.ts Outdated
* Get whether this object was written by replication (replica).
* @return true if this object is a replica
*/
getReplicationIsReplica() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should still also have an || data.replicationInfo.status === 'REPLICA' for cross compatiblity with older code ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now it's just a simple getter.

Comment thread lib/versioning/VersionID.ts
@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented Jun 1, 2026

Conflict

There is a conflict between your branch improvement/ARSN-578/micro-version-id and the
destination branch development/8.4.

Please resolve the conflict on the feature branch (improvement/ARSN-578/micro-version-id).

git fetch && \
git checkout origin/improvement/ARSN-578/micro-version-id && \
git merge origin/development/8.4

Resolve merge conflicts and commit

git push origin HEAD:improvement/ARSN-578/micro-version-id

Comment thread lib/models/ObjectMD.ts Outdated
Comment thread lib/models/ObjectMD.ts
@SylvainSenechal SylvainSenechal force-pushed the improvement/ARSN-578/micro-version-id branch from 2d68077 to 031c259 Compare June 1, 2026 11:37
Comment thread lib/models/ObjectMD.ts
storageType: storageType || '',
dataStoreVersionId: dataStoreVersionId || '',
isNFS,
isReplica: isReplica !== undefined ? isReplica : (status === 'REPLICA' ? true : undefined),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
isReplica: isReplica !== undefined ? isReplica : (status === 'REPLICA' ? true : undefined),
isReplica: isReplica !== undefined ? isReplica : (status === 'REPLICA' ? true : false),

not sure if undefined or false when status not REPLICA

Copy link
Copy Markdown
Contributor Author

@maeldonn maeldonn Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's a boolean i would say false. But if we can save some storage in database, we should use undefined.

@SylvainSenechal SylvainSenechal force-pushed the improvement/ARSN-578/micro-version-id branch 2 times, most recently from 110b0c8 to e166386 Compare June 2, 2026 17:58
@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented Jun 2, 2026

Incorrect fix version

The Fix Version/s in issue ARSN-578 contains:

  • 8.4.4

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 8.4.5

Please check the Fix Version/s of ARSN-578, or the target
branch of this pull request.

Comment thread lib/models/ObjectMD.ts
Comment thread lib/versioning/VersionID.ts
Comment thread lib/versioning/VersionID.ts Outdated
@claude
Copy link
Copy Markdown

claude Bot commented Jun 2, 2026

  • checkCrrCascadeEvent does not validate incomingRaw with isMicroVersionIdComparable, so comparing a legacy 16-char hex microVersionId against a time-ordered ID can produce incorrect stale/proceed results.
    - Add !isMicroVersionIdComparable(incomingRaw) to the guard clause at line 393 of VersionID.ts.
    - getMicroVersionId() return value changed from null to undefined — a silent breaking change for downstream callers checking === null.
    - Document this as a breaking change, or keep returning null for backward compatibility.
    - No tests for isMicroVersionIdComparable and checkCrrCascadeEvent — these new exported functions have four code paths that need coverage.
    - Add unit tests covering non-comparable input, loop, stale, and proceed cases.

    Review by Claude Code

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.71%. Comparing base (a1fa5b4) to head (2c429ab).

Additional details and impacted files
@@                 Coverage Diff                 @@
##           development/8.4    #2628      +/-   ##
===================================================
+ Coverage            73.65%   73.71%   +0.06%     
===================================================
  Files                  223      223              
  Lines                18247    18268      +21     
  Branches              3776     3807      +31     
===================================================
+ Hits                 13440    13467      +27     
+ Misses                4802     4796       -6     
  Partials                 5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

maeldonn added 2 commits June 2, 2026 21:23
Replace the random 8-byte hex microVersionId the
timestamp-ordered identifier matching the versionId scheme, and add
helpers for the crr cascade feature.
Add an optional isReplica flag to ReplicationInfo to distinguish replica
writes from user writes, enabling cascaded CRR.

Issue: ARSN-578
@SylvainSenechal SylvainSenechal force-pushed the improvement/ARSN-578/micro-version-id branch from e166386 to 2c429ab Compare June 2, 2026 19:24
Comment thread lib/models/ObjectMD.ts
@claude
Copy link
Copy Markdown

claude Bot commented Jun 2, 2026

  • getMicroVersionId() return value changed from null to undefined when unset — this is a silent breaking change for any caller using === null
    - Either keep returning null for backward compatibility, or confirm all downstream callers (CloudServer, Backbeat, S3utils) have been updated to handle undefined

    - updateMicroVersionId() signature changed from zero args to two required args (instanceId, replicationGroupId) — coordinated breaking change
    - PR body notes Backbeat and S3utils PRs are "Not done yet"; ensure those are updated before merging this

    Review by Claude Code

* - the legacy 16-char random-hex value written by old ObjectMD code
* - any other string shorter than the minimum valid raw length (27 chars)
*/
export function isMicroVersionIdComparable(id: string | null | undefined): boolean {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function isMicroVersionIdComparable(id: string | null | undefined): boolean {
export function isComparable(id?: string | null): boolean {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the field really be null ? 🤔

* 16-char hex, or otherwise too short) : a legacy incoming ID must never
* produce a false 'stale' or 'loop' event
*/
export function checkCrrCascadeEvent(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function checkCrrCascadeEvent(
export function compare(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'loop' | 'stale' | 'proceed' are specific to CRR not to versionID format. We should just expose a simple compare function in this file

Comment thread lib/models/ObjectMD.ts

setReplicationStatus(status: string) {
this._data.replicationInfo.status = status;
if (status === 'REPLICA') {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is useless since we are not going anymore to set status to 'REPLICA'

Comment thread lib/models/ObjectMD.ts
* @return true if this object is a replica
*/
getReplicationIsReplica(): boolean {
return this._data.replicationInfo.isReplica === true
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if need logic in this getter. Is it used ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-review-retro Identify PR with claude review that have issues (like duplicated comments)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants