refactor: Decouple client.DB from net #2768
Conversation
fredcarle
added
refactor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2768 +/- ##
===========================================
+ Coverage 78.75% 78.95% +0.20%
===========================================
Files 315 315
Lines 23836 23873 +37
===========================================
+ Hits 18771 18848 +77
+ Misses 3685 3654 -31
+ Partials 1380 1371 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 18 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
fredcarle
force-pushed
the
fredcarle/refactor/node-store-dependency
branch
from
d0dee58
to
8d01f2a
Compare
| for _, rep := range replicators { | ||
| schemaMap := make(map[string]struct{}) | ||
| for _, schema := range rep.Schemas { | ||
| schemaMap[schema] = struct{}{} |
There was a problem hiding this comment.
suggestion: One liner comment on the nested for-loops will be handy IMO. Also is missing test coverage.
There was a problem hiding this comment.
The replicator event requires a map so we have to build it. Do we really need a comment for this?
There was a problem hiding this comment.
What info are you looking for here Shahzad? I think a comment here would reduce the readability of the code
There was a problem hiding this comment.
perhaps this is more personal pref, I like comments like: "convert to map for replicator event". But no biggie
|
I do like the changes but there seems to be a massive hit (-4%) to code coverage in this PR:
|
| // P2PTopic is an event that is published when a peer has updated the topics it is subscribed to. | ||
| type P2PTopic struct { | ||
| ToAdd []string | ||
| ToRemove []string | ||
| } | ||
|
|
||
| // PeerInfo is an event that is published when the node has updated its peer info. | ||
| type PeerInfo struct { | ||
| Info peer.AddrInfo | ||
| } | ||
|
|
||
| // Replicator is an event that is published when a replicator is added or updated. | ||
| type Replicator struct { | ||
| // The peer info for the replicator instance. | ||
| Info peer.AddrInfo | ||
| // The map of schema roots that the replicator will receive updates for. | ||
| Schemas map[string]struct{} | ||
| // Docs will receive Updates if new collections have been added to the replicator | ||
| // and those collections have documents to be replicated. | ||
| Docs <-chan Update | ||
| } |
There was a problem hiding this comment.
suggestion: I already expressed my concern about having all possible events gathered here in event package here #2700 (comment).
This is a code-smell. event package should be responsible for sending, receiving, routing event, not on specific event types. It shouldn't event know about ipfs and libp2p.
The structs should go to places where they belong, for example, where the event are supposed to be fired.
I don't see any problem in defining these event in peer package. For example, with P2PTopic it's just db package that fires and listens to the event.
There was a problem hiding this comment.
We can't. The type needs to be known by both the emitter and receiver. If they are defined in their respective "most appropriate" packages, we will end up with circular dependencies. This was already covered when Keenan introduced the change.
There was a problem hiding this comment.
this just means that we have a problem with design and code structure. Moving stuff here is just another work-around.
2 packages can not be dependent of each other, that means whoever is independent can host the event definition.
But it's your call. I believe this PR is a good step towards cleaner code structure overall and I'm fine with keep the events here as a temporary solution. Would be nice to ticket though to keep track of our technical dept.
There was a problem hiding this comment.
Here is an example of why I'm saying this. Right now all of the current events could be moved to the net package. Lets say though that we need to add one to the db package because it's a db specific event and net, amongst other packages, needs to know about that db event. We end up with a situation where net imports db and db imports net.
There was a problem hiding this comment.
If we ever needed an event type to be shared between db and net, we should probably define that type in a third pacakge, probably core. I do agree with Islam that the generic events package should not be coupled to its usecases.
This coupling aside, events could be quite a handy package to use outside of the defra repo (like immutable).
@fredcarle Do you have any objections to defining them in core, or a sub-package of core?
There was a problem hiding this comment.
Just to clarify, when I say everything I mean every event types. Not the bus implementation itself.
There was a problem hiding this comment.
You don't picture us ever needing/wanting internal event types?
There was a problem hiding this comment.
If you mean that the public ones can be in client/event and the internal ones in internal/core/event or something like that then yes I think that's totally fine and probably desired.
There was a problem hiding this comment.
Nice, Islam are you happy with this?
And if so, are you happy with re-orging the events into client/core in another PR (if that's what Fred prefers)? I do see moving them as out of scope here, and if I was the author would rather do it elsewhere.
There was a problem hiding this comment.
yeah, I'm fine with it, also with doing this in a separate PR.
That can't be right. I've seen that in an other PR lately as well. This one for example: #2748. I think it's a codcov bug. |
A lot of the CI in this PR is failing atm, it might be caused by that (failing tests hopefully dont contribute to code coverage) |
fredcarle
force-pushed
the
fredcarle/refactor/node-store-dependency
branch
2 times, most recently
from
3bd9996
to
e54c1ac
Compare
fredcarle
force-pushed
the
fredcarle/refactor/node-store-dependency
branch
from
e2af1e9
to
ae39093
Compare
fredcarle
force-pushed
the
fredcarle/refactor/node-store-dependency
branch
8 times, most recently
from
7a7de1b
to
571cbc9
Compare
fredcarle
force-pushed
the
fredcarle/refactor/node-store-dependency
branch
from
571cbc9
to
0686be2
Compare
fredcarle
force-pushed
the
fredcarle/refactor/node-store-dependency
branch
from
ab6f897
to
44fd3c6
Compare
Relevant issue(s)
Resolves #2767
Description
This PR make the
netpackage no longer dependent onclient.DB. Instead, on creation, the peer takes in a rootstore, a blockstore and an event bus.The p2p collections and replicators have been moved to
db. Most of the code is reused but it now makes use of the event bus to update the peer's related state (pubsub topics and peer connections).Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: