Skip to content
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

bugfix: S3UTILS-110 pass VersionId to PutMetadata route #265

Draft
wants to merge 11 commits into
base: development/1.13
Choose a base branch
from

Conversation

jonathan-gramain
Copy link
Contributor

@jonathan-gramain jonathan-gramain commented Oct 5, 2022

I reverted the crrExistingObjects script to its version 1.4.1 compatible with S3C, then cherry-picked what seems like relevant changes that were applied afterwards on the MongoDB-specific version on top.

On top of this, I then added the actual fix for S3C-6244

jonathan-gramain and others added 11 commits October 4, 2022 14:37
- use ObjectMD model instead of raw parsed JSON object

- update the new microVersionId field before putting the metadata
  changes, to force MongoDB to apply the change and generate an event
  in the oplog

- side fix: call MetadataWrapper.close() so that the script exits
  after completion

(cherry picked from commit 9e97958)
Fix bug on updating metadata. Add 'DEBUG' option to output debug level
information.

(cherry picked from commit 99a8180)
The code used to re-create the replication state, which could break some
fields (esp. dataLocation) or remove some replication backends.

We now simply update the status, which is safer (more future-proof) and
simpler.

Issue: S3UTILS-77
(cherry picked from commit 9df7207)
This is especially needed in case the object was created before
replication is enabled on the bucket.

Issue: S3UTILS-76
(cherry picked from commit ab22723)
Update BackbeatClient with latest schema from BackbeatAPI 8.4 branch
Make sure the `PUT /_/backbeat/metadata` route gets the VersionId
attribute, so that it is not confused in thinking we are attempting to
update the master version.
@bert-e
Copy link
Contributor

bert-e commented Oct 5, 2022

Hello jonathan-gramain,

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.

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Oct 5, 2022

Waiting for approval

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

  • the author

  • 2 peers

const versions = data.DeleteMarkers
? data.Versions.concat(data.DeleteMarkers) : data.Versions;
return _markPending(bucket, versions, err => {
return _markPending(bucket, data.Versions.concat(data.DeleteMarkers), err => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not concat if data.DeleteMarkers is null?
e.g. keep the older code or data.Versions.concat(data.DeleteMarkers || []) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this extra check was introduced in the Zenko version to cope with the change in API (using metadataUtil listing instead of S3), it should not be necessary and 1.4.1 does not have this check, as the SDK always returns an empty array if there is no delete marker.

} = require('async');
const werelogs = require('werelogs');

const { ObjectMD } = require('arsenal').models;
Copy link
Contributor

Choose a reason for hiding this comment

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

this imports arsenal 8 models, which may not be suitable for S3C ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory they should be compatible, but a few fields have been added in 8 (I see content-language for example which does not look to be optional but filled with an empty string by default). While they should not cause trouble in practice I agree that we should be careful and it may be better to avoid using model 8 on S3C.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants