refactor: CRDT merge direction #2016
Conversation
The removed code has no impact on the execution of the delete.
The returned links are never used in the code base and the nodeGetter param is never used within the function.
fredcarle
added
area/crdt
fredcarle
force-pushed
the
fredcarle/refactor/I1066-Incorrect-crdt-merge-direction
branch
from
87e9082 to
9a89494
Compare
Codecov ReportAttention:
@@ Coverage Diff @@
## develop #2016 +/- ##
===========================================
- Coverage 73.97% 73.88% -0.09%
===========================================
Files 248 248
Lines 24822 24801 -21
===========================================
- Hits 18361 18324 -37
- Misses 5205 5218 +13
- Partials 1256 1259 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
fredcarle
force-pushed
the
fredcarle/refactor/I1066-Incorrect-crdt-merge-direction
branch
from
9a89494 to
e6d5da8
Compare
net/process.go
Outdated
| if err != nil { | ||
| return nil, errors.Wrap("failed to decode delta object", err) | ||
| if isComposite { | ||
| bp.composites.PushFront(nd) |
There was a problem hiding this comment.
todo: This feels quite strange, as you are appending to a private property on an object that never reads from it (the only reading of it is done from server.go). Given that this is the only property on this object it makes me wonder why blockProcessor exists as a type at all.
Also strange is that the block in server.go that does read from this, only uses stuff (peer) that is already on this object.
I suggest either removing blockProcessor as a type entirely (and just pass the list around), or move the below block from server.go into this type (into a (bp *blockProcessor) ProcessBlocks() func or similar):
for e := bp.composites.Front(); e != nil; e = e.Next() {
nd := e.Value.(format.Node)
err := s.peer.processBlock(ctx, txn, col, dsKey, nd, "")
if err != nil {
log.ErrorE(
ctx,
"Failed to process block",
err,
logging.NewKV("DocKey", dsKey.DocKey),
logging.NewKV("CID", nd.Cid()),
)
}
}
There was a problem hiding this comment.
I move the block to process.go and simplified things a little more. blockProcessor had more fiels initially but I was able make the change leaner and it got kept around. I added some relevant fields to help pass things around. I like having the blockProcessor type as it keeps the processing under a specific domaine.
There was a problem hiding this comment.
Change looks good, thanks Fred :)
| if j.nodeGetter != nil && j.cid.Defined() { | ||
| cNode, err := j.nodeGetter.Get(p.ctx, j.cid) | ||
| if err != nil { | ||
| log.ErrorE(p.ctx, "Failed to get node", err, logging.NewKV("CID", j.cid)) |
There was a problem hiding this comment.
todo: Please define these errors (and others in other files) in an errors.go file
There was a problem hiding this comment.
They are not returned errors. It's a log message. We don't predefine our log messages at all currently.
There was a problem hiding this comment.
Our log messages are public, just like errors. Most of the time the error messages will be exposed via logs (e.g. CLI/HTTP), the reasons for housing them in a dedicated and standardised file are exactly the same.
There was a problem hiding this comment.
Although there is no value in defining them in the errors.go file. It's not an error that can be handled or compared. It's just a message in a log file.
There was a problem hiding this comment.
I'm ok with a solution like that. I'll leave it as is for now and create a separate PR to take care of log messages.
There was a problem hiding this comment.
It's just a message in a log file.
nitpick: It is an error message in a log file, and indistinguishable to our users from any other 'kind' of error.
I'll leave it as is for now and create a separate PR to take care of log messages
I'm fine with that, but I do think they should live in an errors.go file, because they are errors.
## Relevant issue(s) Resolves sourcenetwork#1066 ## Description This PR change the CRDT merge direction. Originally, when an update arrived over the P2P network, each block was added to the DAG store and merged into the datastore in the order that they were received (newest to oldest block). With this update, we start my receiving all blocks up to the version that we currently have (or until we reach the root) and then we apply the merge from oldest to newest block. This will enable more CRDT types to be added.
Relevant issue(s)
Resolves #1066
Description
This PR change the CRDT merge direction. Originally, when an update arrived over the P2P network, each block was added to the DAG store and merged into the datastore in the order that they were received (newest to oldest block).
With this update, we start my receiving all blocks up to the version that we currently have (or until we reach the root) and then we apply the merge from oldest to newest block. This will enable more CRDT types to be added.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: