Support a single change stream to listen to all op log events #2540

williamlardier · 2024-09-13T13:13:48Z

Listen to all collections but the special ones and the mpu shadow buckets.
Introduce pipeline factories, that will handle all the logic related to pipelines, to simplify other files.
Introduce the UniqueConnector strategy, to handle the new type of pipeline and allocations in a standard way.
Update the handling of old connector: the strategies' & their pipeline factory are used to know if an old bucket should be removed, or kept. This means, if we switch between wildcard and non-wildcard pipelines, we ensure a given bucket is never being listened to more than once.

codecov · 2024-09-13T13:14:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.62%. Comparing base (0a32e66) to head (336ab09).

Additional details and impacted files

Files with missing lines	Coverage Δ
extensions/oplogPopulator/OplogPopulator.js	`88.97% <100.00%> (+0.26%)`	⬆️
...ns/oplogPopulator/OplogPopulatorConfigValidator.js	`66.66% <ø> (ø)`
...Populator/allocationStrategy/AllocationStrategy.js	`100.00% <100.00%> (+16.66%)`	⬆️
...lator/allocationStrategy/RetainBucketsDecorator.js	`100.00% <100.00%> (ø)`
...logPopulator/allocationStrategy/UniqueConnector.js	`100.00% <100.00%> (ø)`
extensions/oplogPopulator/constants.js	`100.00% <ø> (ø)`
extensions/oplogPopulator/modules/Connector.js	`85.10% <100.00%> (-0.16%)`	⬇️
...nsions/oplogPopulator/modules/ConnectorsManager.js	`93.98% <100.00%> (+0.04%)`	⬆️
...pulator/pipeline/MultipleBucketsPipelineFactory.js	`100.00% <100.00%> (ø)`
...ensions/oplogPopulator/pipeline/PipelineFactory.js	`100.00% <100.00%> (ø)`
... and 1 more

... and 8 files with indirect coverage changes

Components	Coverage Δ
Bucket Notification	`66.66% <ø> (ø)`
Core Library	`73.98% <ø> (-0.04%)`	⬇️
Ingestion	`69.24% <ø> (+0.53%)`	⬆️
Lifecycle	`75.00% <ø> (ø)`
Oplog Populator	`84.66% <100.00%> (+1.71%)`	⬆️
Replication	`57.42% <ø> (ø)`
Bucket Scanner	`85.60% <ø> (-0.16%)`	⬇️

@@                 Coverage Diff                 @@
##           development/8.6    #2540      +/-   ##
===================================================
+ Coverage            69.37%   69.62%   +0.24%     
===================================================
  Files                  194      198       +4     
  Lines                12791    12916     +125     
===================================================
+ Hits                  8874     8993     +119     
- Misses                3907     3913       +6     
  Partials                10       10

Flag	Coverage Δ
api:retry	`9.62% <0.00%> (-0.05%)`	⬇️
api:routes	`9.52% <0.00%> (-0.04%)`	⬇️
bucket-scanner	`85.60% <ø> (-0.16%)`	⬇️
ingestion	`12.47% <0.00%> (-0.01%)`	⬇️
lib	`7.52% <0.00%> (-0.04%)`	⬇️
lifecycle	`19.35% <0.00%> (-0.09%)`	⬇️
notification	`0.88% <0.00%> (-0.01%)`	⬇️
replication	`18.89% <0.00%> (-0.08%)`	⬇️
unit	`43.38% <100.00%> (+0.24%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

extensions/oplogPopulator/OplogPopulator.js

francoisferrand

I wonder if this is the right approach... This change is trying to fit this single-change-stream approach in the existing code flow (computing which connector should handle the bucket, etc...), while in this approach there is mostly nothing to do : it is really about setting up one connector on system startup, and just call it a day...

(would still need to keep the reconcile loop, to ensure we restart failed connectors, though... and we may need to consider what to do when changing the overall mode)

I don't know if we should change or how, but I think we really need to dig a bit deeper here.

williamlardier · 2024-09-16T08:24:10Z

@francoisferrand shouldn't we also handle "old" connectors on startup? We can detect them and close them to recreate a single connector, or leave them untouched and handle any new connector in a single one. If we only create one connector, I'm not sure this will work for existing deployments.

Edit: as discussed the current code should cover our tests needs. If a customer needs this approach, we will handle it manually, so no need to handle old connectors here.

bert-e · 2024-09-19T11:51:56Z

Hello williamlardier,

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 · 2024-09-19T11:52:01Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

Issue: BB-602

This moves the logic out of the OplogPopulator file. Issue: BB-602

- The strategy manages a pipeline that listens to all buckets. Issue: BB-602

- Each facroty manages a specific type of pipeline. - We use the pipeline in the allocation strategy. - The Connector now uses the factory from the allocation strategy to create the right pipeline. - The ConnectorsManager now rely on the allocation strategy to decide if an old connector should be kept or not, based on the old connector config, and the current allocation strategy. Issue: BB-602

Issue: BB-602

- This ensures we never listen to a bucket twice - Handling of "transitions", i.e., to ensure there will be no event missed or seen twice, is out of scope, and will be covered by a procedure for now. Issue: BB-602

Issue: BB-602

williamlardier · 2024-10-25T14:05:59Z

/wait
/approve

extensions/oplogPopulator/modules/ConnectorsManager.js

Kerkesni · 2024-11-13T14:00:14Z

extensions/oplogPopulator/allocationStrategy/UniqueConnector.js

+        if (!connectors.length) {
+            return null;
+        }
+        return connectors.find(conn => conn.config.pipeline?.includes(constants.wildCardForAllBuckets)) || null;


Nit: Should we fail or crash when we don't find the connector ? This should never happen looking at the code. Also if we fall in that case and return null a new "wildcard" connector will be created but we're not certain no other connectors are laying around which will be a problem, maybe we should check that ?

francoisferrand · 2024-12-03T08:41:42Z

extensions/oplogPopulator/allocationStrategy/AllocationStrategy.js

+    getOldConnectorBucketList(oldConfig) {
+        const bucketList = this.pipelineFactory.extractBucketsFromConfig(oldConfig);
+        if (this.pipelineFactory.isValid(bucketList)) {
+            return bucketList;
+        }
+        return null;
+    }


this function does not have to do with the allocation strategy : the "validation" could be moved in the pipeline factory

francoisferrand · 2024-12-03T08:44:25Z

extensions/oplogPopulator/allocationStrategy/AllocationStrategy.js

     * @param {Logger} params.logger logger object
     */
    constructor(params) {
+        this._pipelineFactory = params.pipelineFactory;


is the pipeline factory really a part of the strategy?
it seems is is only used as a vessel, to avoid passing an extra parameter to ConnectorManager : but may be cleaner to just keep things separate (lower coupling), and add the parameter where it should be.

williamlardier mentioned this pull request Sep 13, 2024

Support limiting bucket number in connectors and mongodb 6 #2539

Merged

williamlardier force-pushed the improvement/BB-602 branch 2 times, most recently from 31288fd to 4b15b15 Compare September 13, 2024 13:21

williamlardier force-pushed the improvement/BB-601-logic branch from 6df3ea0 to f0a136a Compare September 13, 2024 13:34

williamlardier force-pushed the improvement/BB-602 branch from 4b15b15 to 4c761c4 Compare September 13, 2024 13:35

williamlardier force-pushed the improvement/BB-601-logic branch from f0a136a to f431436 Compare September 13, 2024 13:40

williamlardier force-pushed the improvement/BB-602 branch from 4c761c4 to 434cb0b Compare September 13, 2024 13:40

francoisferrand reviewed Sep 16, 2024

View reviewed changes

extensions/oplogPopulator/OplogPopulator.js Outdated Show resolved Hide resolved

francoisferrand reviewed Sep 16, 2024

View reviewed changes

williamlardier force-pushed the improvement/BB-601-logic branch 16 times, most recently from 7396ddc to a1e9895 Compare September 19, 2024 10:01

Base automatically changed from improvement/BB-601-logic to development/8.6 September 19, 2024 11:51

williamlardier force-pushed the improvement/BB-602 branch 5 times, most recently from d6d5c88 to 22bbd23 Compare October 3, 2024 14:48

williamlardier requested review from Kerkesni and francoisferrand October 4, 2024 07:28

williamlardier force-pushed the improvement/BB-602 branch from 22bbd23 to d882d6e Compare October 4, 2024 07:30

williamlardier added 3 commits October 17, 2024 14:23

Add wildcard constant as a way to detect unique pipelines

ded47b7

Issue: BB-602

Bind the event in the strategy decorator

1eaeaa6

This moves the logic out of the OplogPopulator file. Issue: BB-602

Create the UniqueConnector allocation strategy

da93c69

- The strategy manages a pipeline that listens to all buckets. Issue: BB-602

williamlardier force-pushed the improvement/BB-602 branch 3 times, most recently from 00d2c2c to b9e5be6 Compare October 18, 2024 07:12

williamlardier added 6 commits October 18, 2024 09:22

Use the UniqueConnector when the number of connector in conf is 0

2f6fbde

Issue: BB-602

Delete porevious connectors not matching the strategy

7a3fdd2

- This ensures we never listen to a bucket twice - Handling of "transitions", i.e., to ensure there will be no event missed or seen twice, is out of scope, and will be covered by a procedure for now. Issue: BB-602

Update existing tests

aa0ba29

Issue: BB-602

Rename getOldConnectors to processOldConnectors

6662fc3

Issue: BB-602

Update unit tests

336ab09

Issue: BB-602

williamlardier force-pushed the improvement/BB-602 branch from b9e5be6 to 336ab09 Compare October 18, 2024 07:22

williamlardier marked this pull request as ready for review October 18, 2024 07:22

Kerkesni approved these changes Nov 13, 2024

View reviewed changes

Kerkesni reviewed Nov 13, 2024

View reviewed changes

extensions/oplogPopulator/modules/ConnectorsManager.js Show resolved Hide resolved

Kerkesni reviewed Nov 13, 2024

View reviewed changes

KillianG approved these changes Nov 26, 2024

View reviewed changes

francoisferrand self-assigned this Nov 29, 2024

francoisferrand reviewed Dec 3, 2024

View reviewed changes

francoisferrand approved these changes Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support a single change stream to listen to all op log events #2540

Support a single change stream to listen to all op log events #2540

williamlardier commented Sep 13, 2024 •

edited

Loading

codecov bot commented Sep 13, 2024 •

edited

Loading

francoisferrand left a comment

williamlardier commented Sep 16, 2024 •

edited

Loading

bert-e commented Sep 19, 2024

bert-e commented Sep 19, 2024

williamlardier commented Oct 25, 2024

Kerkesni Nov 13, 2024

francoisferrand Dec 3, 2024

francoisferrand Dec 3, 2024 •

edited

Loading

Support a single change stream to listen to all op log events #2540

Are you sure you want to change the base?

Support a single change stream to listen to all op log events #2540

Conversation

williamlardier commented Sep 13, 2024 • edited Loading

codecov bot commented Sep 13, 2024 • edited Loading

Codecov Report

francoisferrand left a comment

Choose a reason for hiding this comment

williamlardier commented Sep 16, 2024 • edited Loading

bert-e commented Sep 19, 2024

Hello williamlardier,

bert-e commented Sep 19, 2024

Request integration branches

williamlardier commented Oct 25, 2024

Kerkesni Nov 13, 2024

Choose a reason for hiding this comment

francoisferrand Dec 3, 2024

Choose a reason for hiding this comment

francoisferrand Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

williamlardier commented Sep 13, 2024 •

edited

Loading

codecov bot commented Sep 13, 2024 •

edited

Loading

williamlardier commented Sep 16, 2024 •

edited

Loading

francoisferrand Dec 3, 2024 •

edited

Loading