Refactoring to onStreamOpen, onStreamClose, and onStreamCloseWithError to include Node
Nobody has claimed this yet.
Assessment
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Newbie friendliness
- 35/100
- Issue type
- Refactor
- Clarity
- Mostly clear
- Activity status
- Stale
- Tech stack
- grpc, java
- Domain
- backend-api-design
Research direction
Start with the stream observer lifecycle and read the linked pull request discussion, especially the tradeoffs around when onStreamOpen fires. Trace how the last DiscoveryRequest provides Node to onStreamClose and onStreamCloseWithError. Done means the callbacks expose Node where available, close callbacks document nullable Node, and the chosen open-event timing is consistent with the ADS/xDS protocol.
Written by the indexing model from the issue text.
Description
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.
- Dominant language
- Java
- Stars
- 312
- Forks
- 150
- PR merge metrics
- No merged PRs in 30d
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from envoyproxy/java-control-plane
-
Difficulty 2/5 1-3 hours Newbie friendliness 45/100
envoyproxy/java-control-plane#481 ·
-
Difficulty 4/5 3-5 days Newbie friendliness 30/100
envoyproxy/java-control-plane#471 · 1 comment ·
-
Difficulty 4/5 3-5 days Newbie friendliness 45/100
envoyproxy/java-control-plane#463 ·
-
help Open
Difficulty 4/5 3-5 days Newbie friendliness 25/100
envoyproxy/java-control-plane#432 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 45/100
envoyproxy/java-control-plane#411 ·
All issues in envoyproxy/java-control-plane
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 65/100
-
bug
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
elastic/gradle-plugins#157 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
cryptomator/hub#497 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
johanhaleby/occurrent#1120 ·