Upgrade microVersionId to timestamp-ordered format and add isReplica#2628
Upgrade microVersionId to timestamp-ordered format and add isReplica#2628maeldonn wants to merge 2 commits into
Conversation
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
|
LGTM |
caf9a51 to
868cae8
Compare
Review by Claude Code |
868cae8 to
2eae0c5
Compare
|
LGTM |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| this._data.microVersionId = crypto.randomBytes(8).toString('hex'); | ||
| updateMicroVersionId(replicationGroupId: string) { | ||
| this._data.microVersionId = MicroVersionIdUtils.generate(replicationGroupId); | ||
| } |
There was a problem hiding this comment.
updateMicroVersionId is documented with @return itself but doesn't actually return this, unlike every other setter/updater in the class (e.g. setReplicationIsReplica, setReplicationIsNFS). Since this method is being rewritten anyway, this is a good opportunity to fix the mismatch.
| } | |
| updateMicroVersionId(replicationGroupId: string) { | |
| this._data.microVersionId = MicroVersionIdUtils.generate(replicationGroupId); | |
| return this; | |
| } |
— Claude Code
Review by Claude Code |
fbaf3fe to
3160d6b
Compare
| */ | ||
| updateMicroVersionId() { | ||
| this._data.microVersionId = crypto.randomBytes(8).toString('hex'); | ||
| updateMicroVersionId(replicationGroupId: string) { |
There was a problem hiding this comment.
Breaking API change: updateMicroVersionId() now requires a replicationGroupId argument. All downstream callers (CloudServer, Backbeat, etc.) that invoke this method without arguments will fail at compile time. Ensure coordinated updates across dependent repos.
— Claude Code
|
LGTM — one breaking change to flag: |
3160d6b to
5f4839a
Compare
| this._data.microVersionId = crypto.randomBytes(8).toString('hex'); | ||
| updateMicroVersionId(replicationGroupId: string) { | ||
| this._data.microVersionId = MicroVersionIdUtils.generate(replicationGroupId); | ||
| } |
There was a problem hiding this comment.
@return itself in the JSDoc, but the method never returns this. Every other setter in this class (setReplicationIsNFS, setReplicationIsReplica, etc.) returns this for chaining. Since this PR is already editing the method and its JSDoc, either add the missing return this; or drop the @return tag.
| } | |
| updateMicroVersionId(replicationGroupId: string) { | |
| this._data.microVersionId = MicroVersionIdUtils.generate(replicationGroupId); | |
| return this; | |
| } |
— Claude Code
Review by Claude Code |
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
5f4839a to
7be6414
Compare
| this._data.microVersionId = crypto.randomBytes(8).toString('hex'); | ||
| updateMicroVersionId(instanceId: string, replicationGroupId: string) { | ||
| this._data.microVersionId = VersionIDUtils.generateVersionId(instanceId, replicationGroupId); | ||
| } |
There was a problem hiding this comment.
@return itself but the method has no return this; statement — it returns void. Since this JSDoc was updated in this PR, either add return this; for consistency with the other setters, or remove the stale @return tag.
| } | |
| * @return itself | |
| */ | |
| updateMicroVersionId(instanceId: string, replicationGroupId: string) { | |
| this._data.microVersionId = VersionIDUtils.generateVersionId(instanceId, replicationGroupId); | |
| return this; |
— Claude Code
Review by Claude Code |
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