implement peer snapshot client and associated methods #212
Conversation
Signed-off-by: Ricardo Santos <[email protected]>
|
The build failure was not caused by this change. I have fixed the issue so if you rebase on the current branch state your build should now pass. |
This change updates from the Fabric v3.0.0-preview release. Signed-off-by: Mark S. Lewis <[email protected]>
There was a problem hiding this comment.
Very nice clear code. Thank you for the contribution!
I have made some change suggestions in code comments. This really needs some accompanying testing too. An end-to-end test should probably:
- Create a snapshot request for some future block.
- Query pending requests to ensure it was created.
- Cancel the (pending) request.
- Query pending requests again to ensure it was cancelled.
pkg/snapshot/snapshot.go
Outdated
| ) | ||
|
|
||
| // Client is a wrapper around snapshot client | ||
| type Client struct { |
There was a problem hiding this comment.
In the chaincode and discovery packages we use Peer to make it clear to the user which component of the Fabric network the are interacting with, and to which they need to supply a client connection. I think this might be better named as Peer for consistency.
There was a problem hiding this comment.
I agree, I even started writing with it, but only from reading the codebase I wasn't able to understand the naming, but it makes sense to me now. Renamed it to Peer.
pkg/snapshot/snapshot.go
Outdated
| } | ||
|
|
||
| // SubmitSnapshotRequest from a specific peer | ||
| func (s *Client) SubmitSnapshotRequest(ctx context.Context, channelID string, blockNum uint64) error { |
There was a problem hiding this comment.
Since we are in a nicely named snapshot package, perhaps this can be shortened to just SubmitRequest? Alternatively, we could reflect the gRPC service names and call it Generate, although that does not seem as clear to me.
There was a problem hiding this comment.
I agree it's clearer if we remove the Snapshot portion from the client methods.
I also agree that naming it Generate is not really clear, specially since it not always reflects the behavior of what happens in the peer, given that the snapshot might be created in the future.
pkg/snapshot/snapshot.go
Outdated
| id identity.SigningIdentity | ||
| } | ||
|
|
||
| func New(conn grpc.ClientConnInterface, id identity.SigningIdentity) *Client { |
There was a problem hiding this comment.
This would be perfectly named if it was creating a Snapshot, since the package is called snapshot. It might be clearer (and more consistent with other packages in this module) if it names what it is creating. For example, NewPeer.
pkg/snapshot/snapshot.go
Outdated
| } | ||
|
|
||
| // CancelSnapshotRequest from a specific peer | ||
| func (s *Client) CancelSnapshotRequest(ctx context.Context, channelID string) error { |
There was a problem hiding this comment.
Perhaps this can be called just CancelRequest or even Cancel? It should probably mirror the naming of the SubmitRequest method. If that is called SubmitRequest, this makes sense as CancelRequest. If the submit is called Generate (or Submit) then this makes more sense as the shorter Cancel.
pkg/snapshot/snapshot.go
Outdated
| } | ||
|
|
||
| // ListPendingSnapshots from a specific peer | ||
| func (s *Client) ListPendingSnapshots(ctx context.Context, channelID string) error { |
There was a problem hiding this comment.
Maybe we can lose the Snapshots part. ListPending is fine as a name, and matches the CLI naming. I would be tempted to call it QueryPending to match the gRPC service name, and to be more consistent with the naming in the chaincode package, which uses Query... for several methods.
The big problem with this method, which needs to be fixed, is that it queries information but returns nothing. I would return a QueryPendingSnapshotsResponse object rather than the slice of block numbers that it contains, in case that protobuf message gets extended in the future.
There was a problem hiding this comment.
Thanks for pointing this out. I was copying and pasting the outlying structure of the methods and missed it. My bad.
|
I've rebased from the master branch but since I'm not very familiar with git rebasing, I'm not sure if the commit history is correct. Could you please take a look and confirm? |
…ent is connecting to Properly rename New constructor to NewPeer to better relfect the component it is creating Rename Client methods by removing the Snapshots subtr to make it clearer Fix QueryPending snapshots method to return QueryPendingSnapshotsResponse Signed-off-by: Ricardo Santos <[email protected]>
|
Thanks for your feedback. I've just fixed the issues you pointed out and will get started on creating an end-to-end test right away. |
Implement e2e snapshot test Signed-off-by: Ricardo Santos <[email protected]>
Implement friendly wrapper around the proto SnapshotClient