-
Notifications
You must be signed in to change notification settings - Fork 20
Upgrade microVersionId to timestamp-ordered format and add isReplica #2628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development/8.4
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,3 @@ | ||||||
| import * as crypto from 'crypto'; | ||||||
| import * as constants from '../constants'; | ||||||
| import * as VersionIDUtils from '../versioning/VersionID'; | ||||||
| import { VersioningConstants } from '../versioning/constants'; | ||||||
|
|
@@ -32,6 +31,7 @@ export type ReplicationInfo = { | |||||
| storageType: string; | ||||||
| dataStoreVersionId: string; | ||||||
| isNFS?: boolean; | ||||||
| isReplica?: boolean; | ||||||
| }; | ||||||
|
|
||||||
| export type ObjectMDTag = { | ||||||
|
|
@@ -243,6 +243,7 @@ export default class ObjectMD { | |||||
| storageType: '', | ||||||
| dataStoreVersionId: '', | ||||||
| isNFS: undefined, | ||||||
| isReplica: undefined, | ||||||
|
maeldonn marked this conversation as resolved.
|
||||||
| }, | ||||||
| dataStoreName: '', | ||||||
| originOp: '', | ||||||
|
|
@@ -1102,9 +1103,20 @@ export default class ObjectMD { | |||||
| storageType?: string; | ||||||
| dataStoreVersionId?: string; | ||||||
| isNFS?: boolean; | ||||||
| isReplica?: boolean; | ||||||
| }) { | ||||||
| const { status, backends, content, destination, storageClass, role, storageType, dataStoreVersionId, isNFS } = | ||||||
| replicationInfo; | ||||||
| const { | ||||||
| status, | ||||||
| backends, | ||||||
| content, | ||||||
| destination, | ||||||
| storageClass, | ||||||
| role, | ||||||
| storageType, | ||||||
| dataStoreVersionId, | ||||||
| isNFS, | ||||||
| isReplica, | ||||||
| } = replicationInfo; | ||||||
| this._data.replicationInfo = { | ||||||
| status, | ||||||
| backends, | ||||||
|
|
@@ -1115,6 +1127,7 @@ export default class ObjectMD { | |||||
| storageType: storageType || '', | ||||||
| dataStoreVersionId: dataStoreVersionId || '', | ||||||
| isNFS, | ||||||
| isReplica: isReplica !== undefined ? isReplica : (status === 'REPLICA' ? true : undefined), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
not sure if undefined or false when status not REPLICA
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's a boolean i would say |
||||||
| }; | ||||||
| return this; | ||||||
| } | ||||||
|
|
@@ -1130,6 +1143,9 @@ export default class ObjectMD { | |||||
|
|
||||||
| setReplicationStatus(status: string) { | ||||||
| this._data.replicationInfo.status = status; | ||||||
| if (status === 'REPLICA') { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' |
||||||
| this.setReplicationIsReplica(true); | ||||||
| } | ||||||
| return this; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -1151,6 +1167,27 @@ export default class ObjectMD { | |||||
| return !!this._data.replicationInfo.isNFS; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Mark this object as the result of a replication write (replica), | ||||||
| * as opposed to a write originating from a user request. | ||||||
| * | ||||||
| * @param isReplica - true if this object was written by replication | ||||||
| * @return itself | ||||||
| */ | ||||||
| setReplicationIsReplica(isReplica: boolean) { | ||||||
|
maeldonn marked this conversation as resolved.
|
||||||
| this._data.replicationInfo.isReplica = isReplica; | ||||||
| return this; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Get whether this object was written by replication (replica). | ||||||
| * @return true if this object is a replica | ||||||
| */ | ||||||
| getReplicationIsReplica(): boolean { | ||||||
| return this._data.replicationInfo.isReplica === true | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if need logic in this getter. Is it used ? |
||||||
| || this._data.replicationInfo.status === 'REPLICA'; | ||||||
| } | ||||||
|
|
||||||
| setReplicationSiteStatus(site: string, status: string) { | ||||||
| const backend = this._data.replicationInfo.backends.find(o => o.site === site); | ||||||
| if (backend) { | ||||||
|
|
@@ -1327,35 +1364,28 @@ export default class ObjectMD { | |||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Create or update the microVersionId field | ||||||
| * | ||||||
| * This field can be used to force an update in MongoDB. This can | ||||||
| * be needed in the following cases: | ||||||
| * | ||||||
| * - in case no other metadata field changes | ||||||
| * | ||||||
| * - to detect a change when fields change but object version does | ||||||
| * not change e.g. when ingesting a putObjectTagging coming from | ||||||
| * S3C to Zenko | ||||||
| * | ||||||
| * - to manage conflicts during concurrent updates, using | ||||||
| * conditions on the microVersionId field. | ||||||
| * | ||||||
| * It's a field of 16 hexadecimal characters randomly generated | ||||||
| * | ||||||
| * Update the microVersionId | ||||||
| * | ||||||
| * This field can be used to force an update in MongoDB when no other | ||||||
| * metadata field changes, to detect a change for CRR, | ||||||
| * and to manage conflicts during concurrent updates using conditions on this field. | ||||||
| * | ||||||
| * @param instanceId - instance identifier (e.g. config.instanceId) | ||||||
| * @param replicationGroupId - replication group ID (e.g. config.replicationGroupId) | ||||||
| * @return itself | ||||||
| */ | ||||||
| updateMicroVersionId() { | ||||||
| this._data.microVersionId = crypto.randomBytes(8).toString('hex'); | ||||||
| updateMicroVersionId(instanceId: string, replicationGroupId: string) { | ||||||
| this._data.microVersionId = VersionIDUtils.generateVersionId(instanceId, replicationGroupId); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are the instanceID and replicaitionGroupId not usually pick'ed up (in arsenal!) by env variables?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For instance id, we have this in cloudserver so maybe we could : const instanceId = process.env.CLOUDSERVER_INSTANCE_ID || config.instanceId; But for replicationGroupId, it's not an env variable. And this function will also be used by S3UTILS so I think we have to leave it like this |
||||||
| return this; | ||||||
| } | ||||||
|
SylvainSenechal marked this conversation as resolved.
SylvainSenechal marked this conversation as resolved.
SylvainSenechal marked this conversation as resolved.
|
||||||
|
|
||||||
| /** | ||||||
| * Get the microVersionId field, or null if not set | ||||||
| * Get the microVersionId field, or undefined if not set | ||||||
| * | ||||||
| * @return the microVersionId field if exists, or {null} if it does not exist | ||||||
| * @return the microVersionId field if set, or undefined if not | ||||||
| */ | ||||||
| getMicroVersionId() { | ||||||
| return this._data.microVersionId || null; | ||||||
| return this._data.microVersionId || undefined; | ||||||
|
maeldonn marked this conversation as resolved.
SylvainSenechal marked this conversation as resolved.
|
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -369,3 +369,40 @@ export function decode(str: string): string | Error { | |||||
| } | ||||||
| return new Error(`cannot decode str ${str.length}`); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Returns true if a stored (raw) microVersionId is time-ordered and comparable. | ||||||
| * | ||||||
| * Returns false for: | ||||||
| * - null / undefined (field absent) | ||||||
| * - 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 { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the field really be null ? 🤔 |
||||||
| return typeof id === 'string' && id.length >= LEGACY_BASE62_DECODED_LENGTH; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Compare an incoming (raw) microVersionId against the one stored in object | ||||||
| * metadata for CRR cascade loop/stale detection. | ||||||
| * | ||||||
| * Returns 'proceed' whenever either ID is non-comparable (absent, legacy | ||||||
| * 16-char hex, or otherwise too short) : a legacy incoming ID must never | ||||||
| * produce a false 'stale' or 'loop' event | ||||||
| */ | ||||||
| export function checkCrrCascadeEvent( | ||||||
|
SylvainSenechal marked this conversation as resolved.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
| incomingRaw: string, | ||||||
| existingRaw: string | null | undefined, | ||||||
| ): 'loop' | 'stale' | 'proceed' { | ||||||
| if (!isMicroVersionIdComparable(incomingRaw) || !isMicroVersionIdComparable(existingRaw)) { | ||||||
| return 'proceed'; | ||||||
| } | ||||||
| const existing = existingRaw as string; | ||||||
| if (incomingRaw === existing) { | ||||||
| return 'loop'; | ||||||
| } | ||||||
| if (incomingRaw > existing) { | ||||||
| return 'stale'; | ||||||
| } | ||||||
| return 'proceed'; | ||||||
| } | ||||||
|
SylvainSenechal marked this conversation as resolved.
|
||||||
Uh oh!
There was an error while loading. Please reload this page.