java-control-plane icon indicating copy to clipboard operation
java-control-plane copied to clipboard

Refactoring to onStreamOpen, onStreamClose, and onStreamCloseWithError to include Node

Open joeyb opened this issue 7 years ago • 0 comments
trafficstars

Relevant discussion: https://github.com/envoyproxy/java-control-plane/pull/35#discussion_r177099465

Ideally we would have access to the Node during the stream open and close callbacks. Due to the stream observer design, there are some tradeoffs in order to support that.

For onStreamOpen, currently we're triggering that callback outside of the request StreamObserver. At that point, we haven't actually received a DiscoveryRequest yet, so we haven't been given the Node yet. A potential workaround would be to delay the onStreamOpen event until we receive the first request message. In theory there could be an arbitrarily large gap between when the stream is actually opened and when the first request is sent and with this approach we lose the ability to measure that. In practice, is that something that we really care about for this use case? With the ADS/xDS protocol design, the first message is always sent from the client and it occurs immediately after the stream is opened.

For onStreamClose and onStreamCloseWithError, we can just cache the Node from the last request message. Consumers will just need to be aware that the node parameter in this scenario is @Nullable because the stream could close without ever receiving a request.

joeyb avatar Mar 28 '18 14:03 joeyb