Repository navigation
Conversation
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 7 files with indirect coverage changes
@@ Coverage Diff @@
## improvement/BB-906/hashed-connectors-manager #2869 +/- ##
=============================================================================
Coverage 77.41% 77.41%
=============================================================================
Files 214 214
Lines 14657 14681 +24
=============================================================================
+ Hits 11347 11366 +19
- Misses 3300 3305 +5
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
f512379 to
d4b2816
Compare
b02ea60 to
aea4f4f
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
6a15600 to
d555261
Compare
3999fe6 to
7712a12
Compare
7712a12 to
342a7bb
Compare
6f598c8 to
72575e3
Compare
72575e3 to
9258cc1
Compare
9258cc1 to
ce2ef77
Compare
ce2ef77 to
c6bc788
Compare
| */ | ||
| async setup() { | ||
| if (this._config.ingestion === 'hashed') { | ||
| await this._setupHashed(); |
There was a problem hiding this comment.
that is a really hacky way to integrate (and dangerous: most API in this class will not work, since the fields are not set...), when the existing code already implements a whole reconcile loop... Not sure we can change now, though :/
There was a problem hiding this comment.
@francoisferrand not sure to follow you on that 🙏 , can you explain more
There was a problem hiding this comment.
Yes it's intentionally "hacky"/minimal. I didnt want to have an abstraction with classes etc. since the goal is to remove all the code in the next release.
There was a problem hiding this comment.
not sure to follow you on that 🙏 , can you explain more
this basically bypasses most of the code in this class (and all the rest of oplog populator) : almost like we ran 2 completely different oplog populations (if (process.env.NEW_OPLOG_POPULATOR) { require("newCode").start(); } else { require("oldCode").start(); }), with the added extra risk that we still perform half the initialization and use some of the existing bindings (health check...)
the old code already implemented a complete reconciliation process, here we just hack around to require some setup and bypass everything else - and not even cleanly by just starting one implementation or the other.
I didnt want to have an abstraction with classes etc. since the goal is to remove all the code in the next release.
I don't think the abstraction is the problem. What we want to remove (eventually) is the behavior, but the whole abstraction/framework was about splitting responsibility : it was proven to work, and probably flexible enough to address the new approach as well...
| * @throws {InternalError} | ||
| */ | ||
| async _setupHashed() { | ||
| try { |
There was a problem hiding this comment.
not sure what version backbeat currently works with, but certainly fine to drop support for older versions.
However, which versions are we talking about? We don't do very fancy stuff, what requires a specific version?
c6bc788 to
a7c898b
Compare
edd21d0 to
153c045
Compare
153c045 to
38c73fd
Compare
38c73fd to
94effd8
Compare
94effd8 to
4a6f28c
Compare
4a6f28c to
ac2d6b0
Compare
The oplog populator now ingests the whole oplog through N hashed connectors (numberOfConnectors), migrating any existing connector to them. Setting `ingestion` to `perBucket` (EXTENSIONS_OPLOG_POPULATOR_INGESTION) keeps the previous per-bucket connectors; switching back deletes the hashed connectors and loses events in between. - Requires Kafka Connect >= 3.5 (offsets API): migrations wait until it is rolled out. - Setup only schedules the reconciliation, so that probes answer while a migration waits for offsets to flush. Readiness no longer requires the metastore change stream, which hashed ingestion does not use. - Without bucket changes to react to, connectors are reconciled every 15s by default instead of every second. - Connectors start at MongoDB's cluster time, not the pod's clock. Issue: BB-906
ac2d6b0 to
c71bec8
Compare
This branch has not been deployed
4 participants
Hand-written description (took me a while). I tried to do much more hand-written and rountrip work with AI. Let me know if it helps :)
This implements the oplog scaling design for the oplog populator (see https://github.com/scality/citadel/pull/375 for details).
The issue with the previous design was that:
Instead, we listen to all buckets all the time, splitting load across N connectors, with a simple
k = hash(bucket) % N.This is implemented as an opt-out ingestion mode, so enabled by default, but can be disabled. The goal is to drop the previous mode entirely in the short term. (That is why I chose not to have an abstraction over possible modes etc. The goal is to have only one mode left, soon, and delete legacy code.)
This is part of a stack of 7 PRs: the lower ones add the building blocks and the manager, this one wires it in, and #2888 adds monitoring. Review bottom-up.
The general idea is:
hashed, opt-outThe connector name is:
Nandkhashsuch that we can easily spot outdated pipelinesIssue: BB-906