Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Need best practices for constructing GRPC clients documented or in samples #197

Open
davidkel opened this issue Sep 20, 2021 · 10 comments
Open

Need best practices for constructing GRPC clients documented or in samples #197

davidkel opened this issue Sep 20, 2021 · 10 comments
Labels
documentation Improvements or additions to documentation

Comments

@davidkel
Copy link
Contributor

Using just node as a client for now I messed around with changing the parameters to GrpcClient creation to see how helpful it was when something was not done right

  1. Incorrect TLS Cert for peer in org2 when communicating to peer in org1
Error: 14 UNAVAILABLE: No connection established
    at Object.callErrorFromStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call.js:31:26)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client.js:179:52)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:336:141)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:299:181)
    at /home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call-stream.js:145:78
    at processTicksAndRejections (internal/process/task_queues.js:79:11) {
  code: 14,
  details: 'No connection established',
  metadata: Metadata { internalRepr: Map {}, options: {} }
  1. invalid hostname
Error: 14 UNAVAILABLE: Name resolution failed for target dns:localhost2:7051
    at Object.callErrorFromStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call.js:31:26)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client.js:179:52)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:336:141)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:299:181)
    at /home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call-stream.js:145:78
    at processTicksAndRejections (internal/process/task_queues.js:79:11) {
  code: 14,
  details: 'Name resolution failed for target dns:localhost2:7051',
  metadata: Metadata { internalRepr: Map {}, options: {} }
  1. invalid port
Error: 14 UNAVAILABLE: No connection established
    at Object.callErrorFromStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call.js:31:26)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client.js:179:52)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:336:141)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:299:181)
    at /home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call-stream.js:145:78
    at processTicksAndRejections (internal/process/task_queues.js:79:11) {
  code: 14,
  details: 'No connection established',
  metadata: Metadata { internalRepr: Map {}, options: {} }
  1. incorrect grpc override
Error: 14 UNAVAILABLE: No connection established
    at Object.callErrorFromStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call.js:31:26)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client.js:179:52)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:336:141)
    at Object.onReceiveStatus (/home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:299:181)
    at /home/dave/github-mine/fabric-gateway/samples/node/node_modules/@grpc/grpc-js/build/src/call-stream.js:145:78
    at processTicksAndRejections (internal/process/task_queues.js:79:11) {
  code: 14,
  details: 'No connection established',
  metadata: Metadata { internalRepr: Map {}, options: {} }
  1. peer endpoint set to 'www.microsoft.com:7051'
    Application just hangs

It may or may not be possible to improve some of these responses as this is purely down to Grpc for node, but I think at least we should have a timeout for a grpc connection via the waitForReady api ?
@bestbeforetoday
Copy link
Member

Timeouts on calls are not yet properly implemented in the Fabric Gateway client API, and are currently a mixed bag across different client languages, with some implementations using an arbitrary timeout and some not specifying a timeout.

Neither the client code examples at grpc.io nor (as far as I can tell) the generated gRPC client stubs make use of waitForReady on the gRPC client, so I'm not sure this is the right approach for us to be taking. Perhaps a better approach might be to allow the client application to specify gRPC call options for a service invocation, which could include a deadline at which to timeout the call.

Having a default timeout value settable for a Gateway connection might be OK, but I really want to avoid the trap of writing an abstraction of the gRPC API rather than just letting the client application make use of key elements of the gRPC API.

@bestbeforetoday
Copy link
Member

To avoid derailing this documentation issue with implementation of timeouts, I've raised #198

@bestbeforetoday bestbeforetoday added the documentation Improvements or additions to documentation label Sep 21, 2021
@davidkel
Copy link
Contributor Author

davidkel commented Sep 27, 2021 •
edited

@bestbeforetoday Another thought that has occurred to me, we should probably discuss the keepalive options for grpc. The node-sdk users are blissfully unaware that the supplied defaults are pretty good for a lot of K8s services (but not all, eg openshift on Z). Wonder if we should include grpc keepalive values in the samples and also should definitely mention these across all the client sdks.

@mbwhite
Copy link
Member

mbwhite commented Sep 30, 2021

https://stackoverflow.com/questions/57930529/grpc-connection-use-keepalive-or-idletimeout

@bestbeforetoday bestbeforetoday removed their assignment Jan 5, 2022
@mbwhite
Copy link
Member

mbwhite commented Apr 6, 2022

reference for the complexity of gRPC https://stackoverflow.com/questions/64484690/grpc-cpp-how-can-i-check-if-the-rpc-channel-connected-successfully

@mbwhite
Copy link
Member

mbwhite commented Apr 6, 2022

It's worth noting the location when the errors that Dave highlighted above; they will occur on the first actual communication to the peer.

So an application can go through the gateway.connect getNetwork getContract and it's not until the submit/evaluateTransaction that any error is reported.

I believe this can give a misleading impression of what might be wrong - the gateway.connect worked so why is it failing now? This idea then leads you down the wrong path for debugging issues.

As an aside as I was using org1-peer1.vcap.me address this makes any issue look like the port failure. Which is doubly confusing. But not the fault of gRPC.

@bestbeforetoday @andrew-coleman if there was some option for an initial 'heart-beat' on the connect it might be beneficial. in the longer term.

@bestbeforetoday
Copy link
Member

I'm not convinced that a "heartbeat" call on the initial connect is actually helpful. Network or service outages can occur (and be resolved) at any time. The initial heartbeat may fail, but a subsequent invocation (that you can no longer attempt) may have worked. Similarly, your first invocation might succeed and your second might fail.

The confusion seems to stem from the perception that there is a single persistent connection that lives from the point you create the gRPC client connection / client / channel, or at least the point where it is passed to the Gateway connect. As described in the linked information above and the gRPC documentation, this is simply not (necessarily) the case.

@mbwhite
Copy link
Member

mbwhite commented Apr 7, 2022 •
edited

@bestbeforetoday valid points; concede that a heart beat is of minimal (or none at all) use.

The observation of "the perception of.. a single persistent connection" would appear to be central thing here. Initially, I will admit that I thought it was 'different' creating the connection outside and then passing that into the Gateway. However, it's clear now exactly why that is a preferred pattern.

Does the naming of the functions in the SDK used by a client application though help to promote the erroneous perception?

Thinking really of that 'connect' call; is this the wrong word (with the benefit of hindsight of course).

@bestbeforetoday
Copy link
Member

Yeah, "connect" was used to align with the API terminology of the legacy SDKs. Maybe it would have been better differently named here, but that's what we have now and I'm not keen on a breaking change to rename it.

@mbwhite
Copy link
Member

mbwhite commented Apr 7, 2022

When do version 2 of the Gateway SDKs come out :-)

Documentation though we could update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bestbeforetoday @mbwhite @davidkel