Support limiting bucket number in connectors and mongodb 6 #2539

williamlardier · 2024-09-10T09:42:55Z

Observations:

Add a security to not accept more than N buckets in a single pipeline, to avoid exceeding the BSON size limit.
Support mongo6 with immutable strategy for connectors.
Update the allocation strategy to ensure that we do not remove buckets from immutable connectors.
Upgrade handling is not needed: previous pipelines will not be updated if the pipelines are immutable.
Fix an issue where a bucket would be temporarily handled by two connectors: if we remove a bucket from the in-memory Set of the connector, it may be propagated to the pipeline after 5s, so the same bucket might be assigned to another connector in-between, causing some event to be processed twice.
Add metrics on the number of buckets exceed.
Add an alert (warning) when we exceed the configured number of connectors.

bert-e · 2024-09-10T09:42:59Z

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-10T09:43:07Z

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.

codecov · 2024-09-10T09:44:03Z

Codecov Report

Attention: Patch coverage is 93.75000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.33%. Comparing base (73669eb) to head (a1e9895).
Report is 15 commits behind head on development/8.6.

Files with missing lines	Patch %	Lines
extensions/oplogPopulator/OplogPopulatorMetrics.js	50.00%	3 Missing ⚠️
...nsions/oplogPopulator/modules/ConnectorsManager.js	90.00%	2 Missing ⚠️
...Populator/allocationStrategy/AllocationStrategy.js	66.66%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
extensions/oplogPopulator/OplogPopulator.js	`88.70% <100.00%> (+17.41%)`	⬆️
...Populator/allocationStrategy/ImmutableConnector.js	`100.00% <100.00%> (ø)`
...Populator/allocationStrategy/LeastFullConnector.js	`100.00% <100.00%> (ø)`
...lator/allocationStrategy/RetainBucketsDecorator.js	`100.00% <100.00%> (ø)`
extensions/oplogPopulator/constants.js	`100.00% <ø> (ø)`
extensions/oplogPopulator/modules/Allocator.js	`93.75% <100.00%> (+6.25%)`	⬆️
extensions/oplogPopulator/modules/Connector.js	`85.26% <100.00%> (+0.81%)`	⬆️
...Populator/allocationStrategy/AllocationStrategy.js	`83.33% <66.66%> (+8.33%)`	⬆️
...nsions/oplogPopulator/modules/ConnectorsManager.js	`93.93% <90.00%> (+2.56%)`	⬆️
extensions/oplogPopulator/OplogPopulatorMetrics.js	`62.00% <50.00%> (-1.64%)`	⬇️

... and 3 files with indirect coverage changes

Components	Coverage Δ
Bucket Notification	`66.66% <ø> (ø)`
Core Library	`74.00% <ø> (-0.08%)`	⬇️
Ingestion	`68.70% <ø> (-0.54%)`	⬇️
Lifecycle	`74.86% <ø> (ø)`
Oplog Populator	`82.95% <93.75%> (+7.04%)`	⬆️
Replication	`57.42% <ø> (ø)`
Bucket Scanner	`85.76% <ø> (ø)`

@@                 Coverage Diff                 @@
##           development/8.6    #2539      +/-   ##
===================================================
+ Coverage            69.08%   69.33%   +0.25%     
===================================================
  Files                  192      194       +2     
  Lines                12713    12791      +78     
===================================================
+ Hits                  8783     8869      +86     
+ Misses                3920     3912       -8     
  Partials                10       10

Flag	Coverage Δ
api:retry	`9.66% <0.00%> (-0.07%)`	⬇️
api:routes	`9.56% <0.00%> (-0.07%)`	⬇️
bucket-scanner	`85.76% <ø> (ø)`
ingestion	`12.47% <0.00%> (-0.13%)`	⬇️
lib	`7.55% <0.00%> (-0.05%)`	⬇️
lifecycle	`19.39% <0.00%> (-0.13%)`	⬇️
notification	`0.88% <0.00%> (-0.01%)`	⬇️
replication	`18.97% <0.00%> (-0.16%)`	⬇️
unit	`42.94% <93.75%> (+0.50%)`	⬆️

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

.github/workflows/alerts.yaml

extensions/oplogPopulator/OplogPopulator.js

extensions/oplogPopulator/OplogPopulatorConfigValidator.js

extensions/oplogPopulator/allocationStrategy/LeastFullConnector.js

extensions/oplogPopulator/modules/Connector.js

francoisferrand

The approach seems to go against in the "design" of the code, which makes it much harder to separate the concern and understand ("proof") that it is working well... And it may create corner cases.

There may obviously be more in the details, but from afar this change should:

Adding a new ImmutableAllocationStrategy (maybe not needed at first, but would avoid going through the now possibly long list of connectors ; it could also help to handle the "histeresis" that we can have more than 1 bucket if the connector is already like that, but we must always allocate "new" buckets to new connectors)
Updating the Allocator (and AllocationStrategy "contract") to support the new situations (e.g. need to allocate a new connector)
Connector is class to manage a connector: it should not be affected, simply we will never reach a situation where we change the list of buckets (thanks to the new allocator/strategy)
Maybe some change to ConnectorsManager, as it bridges Allocator and Connectors, not sure

issue: BB-601

extensions/oplogPopulator/OplogPopulator.js

extensions/oplogPopulator/constants.js

extensions/oplogPopulator/modules/Connector.js

francoisferrand · 2024-09-19T07:21:41Z

extensions/oplogPopulator/modules/Allocator.js

                await connector.removeBucket(bucket);
+                this.emit('bucket-removed', bucket, connector);


similarly, this should be done the other way to avoid race conditions:

Suggested change

await connector.removeBucket(bucket);

this.emit('bucket-removed', bucket, connector);

this.emit('bucket-removed', bucket, connector);

await connector.removeBucket(bucket);

(to avoid these, maybe we should [in a later PR] refactor the code to make these calls non-async, since currently they do not perform the change...)

francoisferrand · 2024-09-19T07:26:04Z

extensions/oplogPopulator/modules/ConnectorsManager.js

-            } else if (connector.isRunning) {
-                return connector.updatePipeline(true);
+            } else if (connector.isRunning && this._allocationStrategy.canUpdate()) {
+                const isPipelineUpdated = connector.updatePipeline(true);


the function is async, so need to await:

Suggested change

const isPipelineUpdated = connector.updatePipeline(true);

const isPipelineUpdated = await connector.updatePipeline(true);

...however this makes the event be generated after the update was performed, which is not what we want: we need to somehow send the event from the connector (e.g. connector becomes an event generator, or ConnectorManager provides an event generator to the connector)

I added the event in the connector, and I eventually moved both in it, so we can handle all events (destroy & update pipeline) from the connector class, and the event is then propagated to the oplog populator through the connectors manager.
I also used a constant to avoid duplication of the string.

extensions/oplogPopulator/modules/ConnectorsManager.js

francoisferrand · 2024-09-19T09:36:27Z

extensions/oplogPopulator/modules/Connector.js

@@ -185,6 +188,7 @@ class Connector {
        }
        try {
            await this._kafkaConnect.deleteConnector(this._name);
+            this.emit(constants.connectorUpdatedEvent, this);


should be sent before actually deleting the connector

Either way will have flaws, if we send it before, and deletion fails, we loose track of the retained bucket (and no easy way to handle it as we work with event (same with callbacks))

(I will change it before, but just sharing for visibility)

extensions/oplogPopulator/modules/ConnectorsManager.js

extensions/oplogPopulator/OplogPopulator.js

extensions/oplogPopulator/allocationStrategy/RetainBucketsDecorator.js

extensions/oplogPopulator/OplogPopulatorMetrics.js

Issue: BB-601 Rename metric to avoid unnecessary name - also fix a comment Issue: BB-601

Issue: BB-601

williamlardier · 2024-09-19T10:01:59Z

/create_integration_branches

bert-e · 2024-09-19T10:02:37Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/BB-601-logic into
development/8.6
w/8.7/improvement/BB-601-logic will be merged into development/8.7

The following branches will NOT be impacted:

development/7.10
development/7.4
development/7.70
development/8.5

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

bert-e · 2024-09-19T10:02:38Z

Waiting for approval

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

the author
2 peers

The following options are set: create_integration_branches

williamlardier · 2024-09-19T11:51:34Z

/approve

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

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/8.6
✔️ development/8.7

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.5

Please check the status of the associated issue BB-601.

Goodbye williamlardier.

The following options are set: approve, create_integration_branches

williamlardier changed the base branch from development/8.6 to improvement/BB-601 September 10, 2024 09:43

williamlardier force-pushed the improvement/BB-601-logic branch 3 times, most recently from 758ed65 to 45a2e59 Compare September 10, 2024 13:18

williamlardier changed the base branch from improvement/BB-601 to development/8.6 September 10, 2024 13:21

williamlardier force-pushed the improvement/BB-601-logic branch 4 times, most recently from 15a205e to 80261a8 Compare September 11, 2024 09:39

williamlardier changed the title ~~Update logic to support a "1 connector=1 bucket" mode~~ Support limiting bucket number in connectors and mongodb 6 Sep 11, 2024

williamlardier force-pushed the improvement/BB-601-logic branch 2 times, most recently from 74a0cc7 to 7736410 Compare September 11, 2024 11:28

williamlardier marked this pull request as ready for review September 11, 2024 11:46

francoisferrand reviewed Sep 12, 2024

View reviewed changes

francoisferrand requested changes Sep 12, 2024

View reviewed changes

Add alert if the number of connector exceeds the soft limit

f8326dc

issue: BB-601

williamlardier force-pushed the improvement/BB-601-logic branch 3 times, most recently from 36f5611 to 7dc61a2 Compare September 12, 2024 15:34

williamlardier requested a review from francoisferrand September 13, 2024 07:58

williamlardier force-pushed the improvement/BB-601-logic branch 4 times, most recently from f0a136a to f431436 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

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

francoisferrand reviewed Sep 16, 2024

View reviewed changes

extensions/oplogPopulator/modules/Connector.js Outdated Show resolved Hide resolved

williamlardier force-pushed the improvement/BB-601-logic branch from d17510b to 93efc96 Compare September 19, 2024 06:28

francoisferrand reviewed Sep 19, 2024

View reviewed changes

williamlardier force-pushed the improvement/BB-601-logic branch from 1e8a7e7 to bcf782e Compare September 19, 2024 08:47

williamlardier requested a review from francoisferrand September 19, 2024 08:48

francoisferrand reviewed Sep 19, 2024

View reviewed changes

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

francoisferrand reviewed Sep 19, 2024

View reviewed changes

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

francoisferrand reviewed Sep 19, 2024

View reviewed changes

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

Kerkesni reviewed Sep 19, 2024

View reviewed changes

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

francoisferrand approved these changes Sep 19, 2024

View reviewed changes

williamlardier force-pushed the improvement/BB-601-logic branch from bcf782e to ccd7d42 Compare September 19, 2024 09:47

williamlardier requested a review from Kerkesni September 19, 2024 09:47

Kerkesni reviewed Sep 19, 2024

View reviewed changes

extensions/oplogPopulator/allocationStrategy/RetainBucketsDecorator.js Outdated Show resolved Hide resolved

francoisferrand reviewed Sep 19, 2024

View reviewed changes

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

francoisferrand reviewed Sep 19, 2024

View reviewed changes

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

williamlardier force-pushed the improvement/BB-601-logic branch from 44bb359 to 34f5e87 Compare September 19, 2024 09:53

williamlardier requested a review from Kerkesni September 19, 2024 09:53

williamlardier force-pushed the improvement/BB-601-logic branch from 34f5e87 to 7396ddc Compare September 19, 2024 09:59

williamlardier added 2 commits September 19, 2024 12:00

- Also ensure no race condition when handling connector events

c0be9ae

Issue: BB-601 Rename metric to avoid unnecessary name - also fix a comment Issue: BB-601

Bump project to 8.6.49

a1e9895

Issue: BB-601

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

Kerkesni approved these changes Sep 19, 2024

View reviewed changes

bert-e merged commit a1e9895 into development/8.6 Sep 19, 2024
9 checks passed

bert-e deleted the improvement/BB-601-logic branch September 19, 2024 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support limiting bucket number in connectors and mongodb 6 #2539

Support limiting bucket number in connectors and mongodb 6 #2539

williamlardier commented Sep 10, 2024 •

edited

Loading

bert-e commented Sep 10, 2024

bert-e commented Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

francoisferrand left a comment

francoisferrand Sep 19, 2024

francoisferrand Sep 19, 2024

williamlardier Sep 19, 2024

francoisferrand Sep 19, 2024 •

edited

Loading

williamlardier Sep 19, 2024

francoisferrand Sep 19, 2024

williamlardier Sep 19, 2024

williamlardier Sep 19, 2024

williamlardier commented Sep 19, 2024

bert-e commented Sep 19, 2024

bert-e commented Sep 19, 2024

williamlardier commented Sep 19, 2024

bert-e commented Sep 19, 2024

		await connector.removeBucket(bucket);
		this.emit('bucket-removed', bucket, connector);

	const isPipelineUpdated = connector.updatePipeline(true);
	const isPipelineUpdated = await connector.updatePipeline(true);

Support limiting bucket number in connectors and mongodb 6 #2539

Support limiting bucket number in connectors and mongodb 6 #2539

Conversation

williamlardier commented Sep 10, 2024 • edited Loading

bert-e commented Sep 10, 2024

Hello williamlardier,

bert-e commented Sep 10, 2024

Request integration branches

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

francoisferrand left a comment

Choose a reason for hiding this comment

francoisferrand Sep 19, 2024

Choose a reason for hiding this comment

francoisferrand Sep 19, 2024

Choose a reason for hiding this comment

williamlardier Sep 19, 2024

Choose a reason for hiding this comment

francoisferrand Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

williamlardier Sep 19, 2024

Choose a reason for hiding this comment

francoisferrand Sep 19, 2024

Choose a reason for hiding this comment

williamlardier Sep 19, 2024

Choose a reason for hiding this comment

williamlardier Sep 19, 2024

Choose a reason for hiding this comment

williamlardier commented Sep 19, 2024

bert-e commented Sep 19, 2024

Integration data created

bert-e commented Sep 19, 2024

Waiting for approval

williamlardier commented Sep 19, 2024

bert-e commented Sep 19, 2024

williamlardier commented Sep 10, 2024 •

edited

Loading

codecov bot commented Sep 10, 2024 •

edited

Loading

francoisferrand Sep 19, 2024 •

edited

Loading