Consume the ingestion topic from the earliest offset - #2808
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 4 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2808 +/- ##
===================================================
- Coverage 75.76% 75.68% -0.09%
===================================================
Files 200 200
Lines 13922 13923 +1
===================================================
- Hits 10548 10537 -11
- Misses 3364 3376 +12
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
delthas
changed the title
6e46ce1 to
a8b4b49
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
a8b4b49 to
ada9705
Compare
ada9705 to
c865711
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
7cd092f to
955e00b
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
This may create another issue ? E.g. if an OOB location is created, then deleted, then another (OOB) location with the same name is created : I am not sure everything will be clean'ed up (topic, ...), could there be a risk that we re-ingest data from the previous location ?
→ Please double check impact of this architecture change.
→ Can you please check if we have some OOB design docs (here or in citadel), maybe this is discussed already and/or should be added there
note: this is mostly for D/R and not urgent, should land on dev/9.6 ?
DarkIsDude
left a comment
DarkIsDude
left a comment
There was a problem hiding this comment.
I also have concern about the use describe by Francois 🙏
|
Agree on development/9.6 (it just wasn't created when I opened the PR) |
955e00b to
4b3d2d8
Compare
Checked this. The ingestion topic and consumer group are fixed per Zenko instance rather than per location ( The populator does clean its own state per source —
On docs: the OOB design doc exists in both places ( |
Move the BackbeatConsumer options built inline in start() into _getConsumerOptions(), following GarbageCollector and #2788, and cover it: one test on the configuration it maps into options, and one asserting start() builds the consumer from it -- the options being right is only useful if they are the ones actually handed over. No behaviour change. Issue: BB-848
The mongo-processor did not set fromOffset, so auto.offset.reset fell back to librdkafka's default `latest`. BB-831 pinned the consumers where such a loss is permanent and listed this one as having "no known permanent-loss path" -- but LogReader persists its own log position in Zookeeper and advances it after publishing a batch, and the ingestion snapshot phase runs once at source creation, so nothing re-reads that stretch of the raft log. A skipped message means the object metadata is never ingested. Set fromOffset to 'earliest' so such messages are processed late instead of never. Pinned unconditionally, since the D/R metadata sink reuses this processor and loses entries the same way -- see BB-848 for the permanent-loss argument, the group geometry and the replay-safety analysis. Issue: BB-848
4b3d2d8 to
6dbeee7
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
|
Moved to 9.6. |
| this._mProducer.setupProducer(cb); | ||
| } | ||
|
|
||
| _getConsumerOptions() { |
There was a problem hiding this comment.
not needed with the latest change, let's keep the change lean (introducing the function also de-couples the options from consumer, which creates a risk if reused and inheriting incorrect params)
There was a problem hiding this comment.
The code is cleaner and easier to read, IMO better to keep it but open to Thomas's opinion
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
4 participants
MongoQueueProcessordid not setfromOffset, so it inherited librdkafka's defaultlatest: a message produced onto a partition with no committed offset while the group had no member — a rolling update, a crash-restart — is skipped forever once the replacement consumer resolves its start position.BB-831 swept this defect class and left this consumer unpinned, as having "no known permanent-loss path". That does not hold.
LogReaderpersists its own log position in Zookeeper and advances it after publishing a batch, and the ingestion snapshot phase runs once at source creation — so nothing re-reads that stretch of the raft log, and a skipped message means the object metadata is never ingested.Pinned unconditionally rather than behind a D/R mode, since the D/R metadata sink reuses this processor and loses entries the same way. The permanent-loss argument, the group geometry and the replay-safety analysis are on BB-848.
The consumer options move into
_getConsumerOptions()for unit coverage, following #2788.Issue: BB-848