Repository navigation
Let the source location's data processor copy pulled data to the location it targets - #2876
SylvainSenechal wants to merge 1 commit into
Conversation
Hello sylvainsenechal,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 4 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.6 #2876 +/- ##
===================================================
- Coverage 81.74% 81.58% -0.16%
===================================================
Files 217 217
Lines 45569 45593 +24
===================================================
- Hits 37250 37199 -51
- Misses 8319 8394 +75
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
c8a6160 to
030ca9f
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
26f607b to
d43e81a
Compare
SylvainSenechal
changed the title
bab8486 to
2f1f046
Compare
There was a problem hiding this comment.
not sure about this PR :
- if we keep using the current way the code works, the message shoudl be processed by the "default/standard" location queue processor (like transitions to that location) - which we probably don't have
- here you tweak every place where we use 'location', to work around it...and need to introduce fromLocatin.
2 things:
- should we process pullReplication in the destination processor or the source one? even if it worked, I agree we should probably keep it in the processor dedicated to that location
- instead of "reversing" the parameters everywhere, maybe we could just fix the message that is sent: set
toLocationto the "source" location, so everything works naturally. And we keep one difference in one place, i.e. where we do the copy and which is already different (and may need to tweak the followup message we send to "update" the storage class)
Both your approach and the one I draft most likely work, but not sure why I am a bit unconfortable with the one you implemented :-/ (the other has one straightforward and explicit inconsistency -i.e. we field is not named correctly- but seems to follow the code more naturally, for afar...)
What do you think?
| // Each data processor performs the copies to its own site, plus the copies | ||
| // from its site when that site is remote (pull replication): it is the only | ||
| // one that can read from there. | ||
| const fromLocation = actionEntry.getAttribute('fromLocation'); |
There was a problem hiding this comment.
we have already in the message an information which explicitely indicates if this is pull-replication or transition:
originLabel: PULL_REPLICATION // 'pullReplication'
originLabel: 'lifecycle'Should we not use this condition instead?
| const fromLocation = actionEntry.getAttribute('fromLocation'); | |
| const loc = actionEntry.getAttribute('origin') === 'pullReplication' ? actionEntry.getAttribute('fromLocation') : actionEntry.getAttribute('toLocation') |
2f1f046 to
bcb4671
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
I applied your two suggestions, using PULL_REPLICATION to check if we are in a pull replication scenario, and also the refactoring with a new isPullReplication method on the Action class. |
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 |
bcb4671 to
9afd940
Compare
| attempt: params.attempt, | ||
| lastModified: params.lastModified, | ||
| }) | ||
| .setAttribute('fromLocation', params.fromLocation) |
There was a problem hiding this comment.
instead of fromLocation, we should add a site attribute which is either equal to fromLocation (for pull replication) or toLocation (for transition)
→ allows to easily filter in QueuePopulator, and can be used in metrics
(needs a fallback to toLocation attribute, for "old" messages)
| actionEntry.getAttribute('metrics'); | ||
| ReplicationMetrics.onReplicationProcessed( | ||
| origin, fromLocation, this.site, contentLength, | ||
| origin, fromLocation, this._getDestination(actionEntry).location, contentLength, |
There was a problem hiding this comment.
getattribute("toLocatoin")
| @@ -318,7 +334,7 @@ class CopyLocationTask extends BackbeatTask { | |||
| const { origin, fromLocation, contentLength } = | |||
| actionEntry.getAttribute('metrics'); | |||
| ReplicationMetrics.onReplicationProcessed( | |||
There was a problem hiding this comment.
the Lifecycle metrics (for PullReplication) should probably use the "site" attribute (i.e. fromLocation in pull replication) : let's create a followup ticket to address this thoroughly (if affects not only CopyLocationTask, but also QueueProcessor and TransitionStatusProcessor)
There was a problem hiding this comment.
Yeah I created a ticket
| if (actionEntry.getAttribute('toLocation') === this.site) { | ||
| // Each data processor performs the copies to its own site, plus the pull | ||
| // replications from its site: it is the only one that can read from there. | ||
| const siteInCharge = actionEntry.isPullReplication() ? |
There was a problem hiding this comment.
route based on site attribute (fallback to toLocation attribute)
| /** | ||
| * A pull replication copies data from a remote location (fromLocation) to a | ||
| * location of this Zenko (toLocation) | ||
| * | ||
| * @return {boolean} true if the action is a pull replication | ||
| */ | ||
| isPullReplication() { | ||
| return this.getContextAttribute('origin') === pullReplicationOrigin; | ||
| } | ||
|
|
There was a problem hiding this comment.
should not be needed : metrics.origin is not used for "deciding", but only to set on metrics
| /** | |
| * A pull replication copies data from a remote location (fromLocation) to a | |
| * location of this Zenko (toLocation) | |
| * | |
| * @return {boolean} true if the action is a pull replication | |
| */ | |
| isPullReplication() { | |
| return this.getContextAttribute('origin') === pullReplicationOrigin; | |
| } |
| // extra round-trip is a bigger overhead than the bandwidth saved | ||
| replicationExpectContinueThreshold: 1024 * 1024, // 1 mb | ||
| // origin of the actions that copy data pulled from a remote location | ||
| pullReplicationOrigin: 'pullReplication', |
There was a problem hiding this comment.
may not be needed anymore (and keep the PULL_REPLICATION metrics)
| const location = actionEntry.getAttribute('toLocation'); | ||
| if (actionEntry.isPullReplication()) { | ||
| return { location, storageType: locations[location]?.type }; | ||
| } | ||
| return { location, storageType: this.destType }; |
There was a problem hiding this comment.
| const location = actionEntry.getAttribute('toLocation'); | |
| if (actionEntry.isPullReplication()) { | |
| return { location, storageType: locations[location]?.type }; | |
| } | |
| return { location, storageType: this.destType }; | |
| const location = actionEntry.getAttribute('toLocation'); | |
| if (location == this.site) { | |
| return { location, storageType: this.destType }; | |
| } | |
| return { location, storageType: locations[location]?.type }; |
9afd940 to
2bae062
Compare
| eTag: params.eTag, | ||
| lastModified: params.lastModified, | ||
| toLocation: params.site, | ||
| site: params.site, |
There was a problem hiding this comment.
This isnt 100$ mandatory but I added it because otherwise, in createCopyLocationAction, we would've had to use a fallback site = params.site || params.toLocation, instead of site = params.site which I prefer
2bae062 to
a0a99db
Compare
a0a99db to
c68f2a2
Compare
| @@ -39,6 +39,21 @@ class CopyLocationTask extends BackbeatTask { | |||
| return this.destConfig?.replicationEndpoint?.type; | |||
There was a problem hiding this comment.
this function should probably be fixed (less changes) or dropped (you replaced multiple -hopefully all- calls)
There was a problem hiding this comment.
yeah I'm inlining it
…it targets Issue: BB-913
c68f2a2 to
d6bc359
Compare
With pull replication, the sink copies each object's data from a remote source location (for example dr-source) into a local location (for example us-east-1). The copy is a copyLocation action with fromLocation = the source and toLocation = the local target.
Two problems in the data processor:
The copy was never performed. Backbeat has one data processor per replication site (a remote location such as dr-source, or a cloud location), and each data processor only performs the copies whose target is its own site. Here the target is the local location (us-east-1). A local location is not a replication site, so it has no data processor, and no data processor performed the copy. The data processor that should perform it is the one of the source location (dr-source): it's the only one with that location's address and credentials, so the only one able to read the data.
Once given to the data processor of dr-source, the copy would go to the wrong place. A data processor always writes the copied data to its own site, because until now its own site was always the target of the copy. With pull replication that's no longer true: the data processor of dr-source performs the copy, but the target is us-east-1. Writing "to its own site" would send the data back to dr-source instead of us-east-1. Cloudserver also needs the storage type of the location it writes to, and that type wasn't sent. So the data processor must write to the target of the copy (us-east-1), with that location's type.
Issue: BB-913