Skip to content

ARSN-617: clean-read filter hiding non-localized versions - #2688

Open
DarkIsDude wants to merge 3 commits into
development/8.5from
improvement/ARSN-617/clean-read-filter
Open

ARSN-617: clean-read filter hiding non-localized versions#2688
DarkIsDude wants to merge 3 commits into
development/8.5from
improvement/ARSN-617/clean-read-filter

Conversation

@DarkIsDude

@DarkIsDude DarkIsDude commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Context

https://scality.atlassian.net/browse/ARSN-617

In a clean room, metadata is replicated before the object data is copied locally: until then the version's dataStoreName still points at the remote production site. Such a version is non-localized and must not be visible to clients.

What this adds

A per-call hideNonLocalizedVersions flag on the metadata read and listing APIs (getObject, getObjects, getBucketAndObject, listObject, listMultipartUploads).

Notes for ARSN-618

ARSN-618 answers the same "which version is current" question at write time, unconditionally, and can reuse the condition helpers and the getLatestVersion filter parameter added here. Read-time resolution stays necessary for entries that never traverse the write path (mongodump/mongorestore bootstrap and re-bootstrap loads); since the design rejected read-time master filtering, the design and ARSN-618 should record that it now exists. We can also decide to drop it as this use case should not exist in a DR architecture.

@bert-e

bert-e commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Hello darkisdude,

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.
/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

This comment was marked as resolved.

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.73%. Comparing base (0bbe970) to head (3f20f15).
⚠️ Report is 1 commits behind head on development/8.5.

Files with missing lines Patch % Lines
lib/storage/metadata/mongoclient/readStream.js 73.07% 7 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           development/8.5    #2688      +/-   ##
===================================================
+ Coverage            74.65%   74.73%   +0.07%     
===================================================
  Files                  227      227              
  Lines                18606    18631      +25     
  Branches              3850     3893      +43     
===================================================
+ Hits                 13890    13923      +33     
+ Misses                4711     4703       -8     
  Partials                 5        5              

☔ View full report in Codecov by Harness.
📢 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.

@DarkIsDude DarkIsDude self-assigned this Aug 24, 2026
@bert-e

This comment was marked as resolved.

@bert-e

This comment was marked as resolved.

@bert-e

bert-e commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Waiting for approval

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

  • the author

  • 2 peers

@DarkIsDude
DarkIsDude marked this pull request as ready for review August 31, 2026 09:11
@DarkIsDude
DarkIsDude requested review from a team, SylvainSenechal, delthas and maeldonn August 31, 2026 09:11
@maeldonn

This comment was marked as resolved.

@DarkIsDude
DarkIsDude force-pushed the improvement/ARSN-617/clean-read-filter branch from ae474fa to cad4307 Compare August 31, 2026 13:02
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/Mongoclient.md Outdated
Comment thread lib/storage/metadata/mongoclient/Mongoclient.md Outdated
Comment thread lib/storage/metadata/mongoclient/Mongoclient.md Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/MetadataWrapper.js Outdated
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
Comment thread lib/storage/metadata/MetadataWrapper.js Outdated
@bert-e

bert-e commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Waiting for approval

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

  • the author

  • 2 peers

The following reviewers are expecting changes from the author, or must review again:

@bert-e

bert-e commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Incorrect fix version

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

  • 8.6.0

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

  • 8.5.16

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

Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts
Comment thread lib/storage/metadata/mongoclient/MongoClientInterface.ts Outdated
@DarkIsDude
DarkIsDude force-pushed the improvement/ARSN-617/clean-read-filter branch from ed25b4e to 3f941a1 Compare September 9, 2026 14:32
@maeldonn

Copy link
Copy Markdown
Contributor

This PR is very difficult to review.

  • Prettier formatting: Please group all formatting into a single initial commit (or ideally, a separate stacked PR).
  • History cleanup: The first commit modifies a file that doesn't actually show changes in the final diff. We should clean up the git history whenever there are large, unnecessary changes like this.

Run prettier on the files ARSN-617 modifies, so that the following
commits carry no formatting noise.

Issue: ARSN-617
In a clean-room D/R deployment, the metadata is replicated before the
data is copied locally, so a version can still point at the remote
source site (`locations[objMD.dataStoreName].isCRR`). Add a per-call
`hideNonLocalizedVersions` flag on the metadata read and listing APIs
to keep those versions out of the client's view, so that any backend
can implement the same contract.

MongoClientInterface builds the filter once from the `locations`
configuration and applies it in the query itself, so that listing
limits and truncation stay correct. Only the version keys are
filtered: a master always points at a localized version, and
resolving an absent or placeholder master recomputes it from the
localized versions only, whatever the caller's view.

MetadataWrapper sets the flag on every read and listing call when the
deployment enables it, and logs an error leaving it off when the
backend does not advertise the support.

Issue: ARSN-617
Unit-test the MongoDB filter and the master resolution against a
stubbed collection, the MetadataWrapper wiring against a stubbed
backend, and run the whole read and listing surface against a real
MongoDB in the functional suite.

Issue: ARSN-617
@DarkIsDude
DarkIsDude force-pushed the improvement/ARSN-617/clean-read-filter branch from 4e0f006 to 3f20f15 Compare September 10, 2026 13:02
@DarkIsDude

Copy link
Copy Markdown
Contributor Author

This PR is very difficult to review.

* Prettier formatting: Please group all formatting into a single initial commit (or ideally, a separate stacked PR).

* History cleanup: The first commit modifies a file that doesn't actually show changes in the final diff. We should clean up the git history whenever there are large, unnecessary changes like this.

I'm not sure it's in our guidelines to have one dedicated commit for all prettier. The guidelines is put prettier in dedicated commit. Several if needed as you can work from one file to a new file during dev time. Anyway, Claude can rework the history easily so it's done. Do you think we could have split this PR in several part ?

@maeldonn maeldonn left a comment

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.

LGTM, left a few minor nits. Logic looks fine, but since I'm not an expert in this area, please wait for a third review from the team.

private replicationGroupId: string;
private database: string;
private isLocationTransient: Function;
private nonLocalizedQuery: object | null;

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
private nonLocalizedQuery: object | null;
private nonLocalizedQuery: Filter<ObjectMetastoreDocument> | null;

c: Collection<ObjectMetastoreDocument>,
objName: string,
vFormat: string,
nonLocalizedFilter: object | null,

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.

nit: could be refactored with an option object

Comment on lines +1733 to +1737
* @param {Object | null} nonLocalizedFilter query fragment hiding the
* non-localized versions. Always the deployment filter, never the caller's
* view: the version returned stands in for the master, the newest localized
* version. `null` filters nothing: no `isCRR` location, or a write path
* waiting on ARSN-618

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
* @param {Object | null} nonLocalizedFilter query fragment hiding the
* non-localized versions. Always the deployment filter, never the caller's
* view: the version returned stands in for the master, the newest localized
* version. `null` filters nothing: no `isCRR` location, or a write path
* waiting on ARSN-618
* @param {Object | null} nonLocalizedFilter query fragment hiding the non-localized
* versions, `null` to filter nothing.

const getLatestVersion = this.getLatestVersion;
const nonLocalizedFilter = params.hideNonLocalizedVersions ? this.nonLocalizedQuery : null;
// the master is the newest localized version, whatever the caller's view
const phdResolutionFilter = this.nonLocalizedQuery;

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.

What is PHD ?

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.

Master PlaceHolDer (IIRC when we delete the latest version and must repair the master to point to the previous latest version)

// or last version is delete marker
if (!doc || doc.value.isPHD) {
this.getLatestVersion(c, objName, vFormat, log, (err, value?) => {
// the master is the newest localized version, whatever the caller's view

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.

this comment is duplicated, does it mean we should do something ?

@delthas delthas left a comment

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.

LGTM code-style wise but this is quite subtle so needs a review from Francois.

(Leaving the approves to Mael and Francois)

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