-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: P2P client interface #1924
Changes from 21 commits
022af8f
a49626a
346fbce
55e2f21
1c3f4e1
175c5a8
292150c
13f2fff
1cfdda3
c55ea7e
1ec5f38
e5b7b53
eb04aed
ab11b65
78ac2cb
f3887a4
5c2d60b
2a01990
7e6bb4f
94a7b8b
592e411
687f0e2
fa6b67e
8cca432
4be14e7
edb9249
b907f30
25139aa
580f016
4532bfd
6ec4577
76c9b5f
03d2a6e
a96746c
b967436
abf2af3
40b2603
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,17 @@ package client | |
|
||
import ( | ||
"context" | ||
|
||
"github.com/libp2p/go-libp2p/core/peer" | ||
) | ||
|
||
// P2P is a peer connected database implementation. | ||
type P2P interface { | ||
DB | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. todo: It took me a while to figure it out, but you have removed functionality here. I'm not sure we wish to lose this, and this change should be made very visible and open to discussion with the team. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I discussed this with @fredcarle and forgot to make a note. We came to the conclusion that peer operations only make sense when done implicitly. It would be pretty difficult to have the transaction also rollback the peer connections that happen during these operations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not losing functionality. We are removing the need for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I do disagree with this, with the obvious example being during an update to a new application version - there may be several of these operations (plus other stuff, like schema updates etc) and if any one of those operations fails, they should all fail so that the update is not partially applied. I will not push back against the both of you on this, but I do think explicit transactions here are useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fred and I spoke about this last week, currently (in develop), the P2P interface only looks like it supports explicit transactions - it doesn't actually respect them. This PR fixes this by making the interface reflect what is actually supported. I'm happy with the change, it is better than what was before :) |
||
|
||
// PeerInfo returns the p2p host id and listening addresses. | ||
PeerInfo() peer.AddrInfo | ||
|
||
// SetReplicator adds a replicator to the persisted list or adds | ||
// schemas if the replicator already exists. | ||
SetReplicator(ctx context.Context, rep Replicator) error | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Should this (and
Long
), not now readDelete replicators...
? It looks like it can now delete multiple.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are referring to deleting multiple collections replicated to a single peer you are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: Please change the short/long wording to reflect this :)