Conversation
toReplicaEntry cloned the source entry and only updated the targeted backend, so the destination metadata inherited the statuses of the source's other destinations, frozen at queue time and never refreshed. Keep the targeted backend alone, copying it since clone() shares replicationInfo with the source. Issue: BB-875
Hello maeldonn,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
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files
... and 5 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2845 +/- ##
===================================================
- Coverage 75.72% 75.48% -0.25%
===================================================
Files 200 200
Lines 13926 13928 +2
===================================================
- Hits 10545 10513 -32
- Misses 3371 3405 +34
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| toReplicaEntry(backend) { | ||
| const newEntry = this.clone(); | ||
|
|
||
| // clone() shares replicationInfo with the source: detach before mutating. |
There was a problem hiding this comment.
should this be fixed in clone() instead? Or are there places where we rely on that shallow cloning behavior?
There was a problem hiding this comment.
Needs more investigation but AI is hinting at directly changing Arsenal
|
|
||
| // clone() shares replicationInfo with the source: detach before mutating. | ||
| newEntry.setReplicationInfo(this.getReplicationInfo()); | ||
| const matched = newEntry._findBackend(backend); |
There was a problem hiding this comment.
why should we send this: our "local" replication status makes no sense on the remote site, should not even be sent ?
There was a problem hiding this comment.
I'm guessing it's important since the pr and jira ticket were probably created after an issue was found 🤔 I think the current implementation is messing up with collectResponseHeader in cloudserver, right @maeldonn ?
| @@ -232,9 +232,15 @@ class ObjectQueueEntry extends ObjectMD { | |||
|
|
|||
| toReplicaEntry(backend) { | |||
There was a problem hiding this comment.
I think in the end the required change is not that complicated, but it took me a fair bit of discussion with ai to understand whats happening and why we are doing this change.
The comment "// clone() shares replicationInfo with the source: detach before mutating." didn't really help me understand what was happening
I think the comment can be more straightforward, if I understood correctly, the issue is :
A source replication site can now be used to replicate to multiple destination. When creating the destination entry, we only care about writing into that destinationEntry the data about where it was replicated, and not the other site, thus the findBackend and matched new thing.
So if I understood correctly, I think it should be more made more clear by the comment .
As always, I think it's not helping that the function has variables named replicationBackendS(its a location?)/site, there is also another setReplicationBackends without an s
Anyways, I think this probably just needs one more comment around findBackend to explain why and thats it
4 participants
toReplicaEntry cloned the source entry and only updated the targeted backend, so the destination metadata inherited the statuses of the source's other destinations, frozen at queue time and never refreshed. Keep the targeted backend alone, copying it since clone() shares replicationInfo with the source.
Issue: BB-875