TaskScheduler: include the bucket and the eTag in the action dedupe key - #2798
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 2 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.6 #2798 +/- ##
===================================================
- Coverage 75.77% 75.57% -0.20%
===================================================
Files 200 200
Lines 13922 13927 +5
===================================================
- Hits 10549 10526 -23
- Misses 3363 3391 +28
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:
|
dc3d703 to
e307e22
Compare
|
Reading back Francois review, whats happens with old data and new data ? I mean, can we have conflicts and/or dedup not working because the key changes at deployment time ? |
77767b3 to
04da66b
Compare
No conflict is possible: the dedupe cache is per-process and in-memory, and it only holds tasks that are currently queued or in flight. Keys are computed at push time, from the message, by whichever process consumes it — they are never persisted, and never compared between processes. A process that restarts or is upgraded starts with an empty cache. Mixed producer versions are not a problem either. Nothing has ever written The worst case during a rolling upgrade is that an old and a new process each deduplicate within their own window, so a duplicate pair could be processed twice instead of once. That is the at-least-once behaviour the pipeline already handles: |
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 |
04da66b to
56026f5
Compare
delthas
changed the title
|
Moved to 9.6. |
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: |
56026f5 to
1db1514
Compare
The queue processor pushed the entries of both its consumers into a single task scheduler: the replication entries of the replication topic, and the copyLocation actions of the data mover topic. Sharing it makes tasks of the two kinds serialize on the same queue key and, worse, deduplicate against each other, although they are distinct work. Give each consumer its own scheduler, so that a dedupe cache only ever holds one kind of entry. Issue: BB-810
The TaskScheduler dedupe key of an ActionQueueEntry omitted the bucket, so two actions targeting the same object key in different buckets were considered duplicates, and the second one was silently dropped. Its third component read target.contentMd5, which no producer has ever set: the content of a copyLocation action is carried by target.eTag. The key of the transition of an object in a non-versioned bucket thus degenerated to `<objectKey>::undefined`, and such an object overwritten with new contents also collided with its own pending action. Build the key from the bucket, the version and the eTag, stripped of its quotes so that the same content always yields the same key. Issue: BB-810
Dropping a task on a dedupe key match was so far entirely silent, which leaves no trace to diagnose from when a task is skipped wrongly. The logger takes the constructor slot of a concurrency parameter that TaskScheduler never had: that limit is enforced by the consumer, in BackbeatConsumer._getAvailableSlotsInPipeline(). Issue: BB-810
1db1514 to
94795a3
Compare
|
Moved each consumer to its own scheduler and dropped the prefix. (First commit). |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
|
/approve |
|
!done 4h |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
This pull request does not target the following hotfix branch(es) so they
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue BB-810. Goodbye delthas. |
|
|
4 participants
The
TaskSchedulerdedupe key of anActionQueueEntryomitted the bucket, so two actions targeting the same object key in different buckets were considered duplicates and the second one was silently dropped — it is acknowledged and its offset committed, so nothing replays it.Two changes, in this order:
1. A distinct task scheduler for the data mover. A queue processor pushed the entries of both its consumers into one scheduler — replication entries from the replication topic (
QueueProcessor.js:936) andcopyLocationactions from the data-mover topic (:993) — so entries of the two kinds serialized on the same queue key and, worse, deduplicated against each other, although they are distinct work. Each consumer now gets its own. Note this also means a replication entry and acopyLocationon the samebucket/keyare no longer serialized against each other; that guard was weak, as_applyTransitionRulealready skips objects whose replication status isPENDING/PROCESSING/FAILED.2. The dedupe key itself. What it is made of, before and after:
index.htmlin two buckets gave the same key, and the second action was dropped.undefinedfor objects in non-versioned buckets, hence the eTag below.contentMd5, alwaysundefinedtarget.contentMd5, in any version: the content of acopyLocationaction is carried bytarget.eTag, so that component was dead and the read is now gone. Without itbucket/keyis a mutable identity — a non-versioned object overwritten between two scans collides with its own pending action, and the pending one then failsCopyLocationTask._checkObjectState(:845-853) with "object contents have changed", so neither action transitions the object. Quotes are stripped so the same content always yields the same key.toLocationprocessDataMoverEntryonly builds a task whentoLocation === this.site(:980), so it is constant within a given scheduler.The
ObjectQueueEntrybranch is unchanged: it already carried the bucket (viagetCanonicalKey()) and a realcontentMd5.Nothing to migrate: a dedupe cache is per scheduler and in-memory, cleared when the task ends, and the key is derived from attributes already published.
Tasks skipped by deduplication are now logged, which was so far silent. The logger takes the constructor slot of a concurrency parameter that
TaskSchedulernever had — that limit is enforced by the consumer, inBackbeatConsumer._getAvailableSlotsInPipeline().Follow-up in BB-855: rather than letting a stale action run and fail its state check, deduplication could replace the queued task with the newer one and keep only the most up-to-date message. That needs
TaskSchedulerto support replacing a queued task, which it cannot do today.Issue: BB-810