Issue #13043 - review of websocket Flushers for 12.1 #13250
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
| private boolean _upstreamDemand = false; | ||
| private boolean _downstreamDemand = false; |
There was a problem hiding this comment.
probably a little comment to clarify which direction upstream and downstream are.
| { | ||
| _emitFrame = emitFrame; | ||
| } | ||
|
|
||
| @Override | ||
| public void setNextDemand(DemandChain nextDemand) |
There was a problem hiding this comment.
Should "Next" be "downstream"? Or if next/prev makes sense, then use them instead of upstream/downstream ?
| } | ||
|
|
||
| /** | ||
| * This is used by the downstream layer to demand a new frame to be processed in {@link #handle(Frame, Callback, boolean)}. |
There was a problem hiding this comment.
Oh - I would have called the layer calling demand upstream, as it is the layer above.
Hmmmm maybe I suggested bad names for you???
There was a problem hiding this comment.
I actually think you meant upstream
There was a problem hiding this comment.
I was thinking that the most upstream would be the WebSocketConnection as the source of the frames, and the most downstream would be the application demanding the frames. In which case this method would correspond to the downstream demand.
But if you think about the demand itself originating from the application then that would be the most upstream.
So I could just reverse all the naming of all my upstream/downstreams.
There was a problem hiding this comment.
@lachlan-roberts Let me think... I could be confused myself...
In this case, data is flowing from the network to the application, so in one sense the "stream" is flowing from low layers down to the application, so the application calling demand would be "downstream" as you said in at least one of your comments.
But more typically (and chatGPT backs me here) upstream is towards a higher layer of the protocol stack, so the application and/or layer above calling demand is "upstream".
Needless to say, the most important thing is to be internally consistent, and I don't think you current comments are. Also you have used "Next" in at least once circumstance, which is a mixed metaphor.
So pick a pair: upstream/downstream; downstream/upstream; prev/next; above/below; etc. and then explain the metaphor in the opening comments apply it rigorously to all code and comments
There was a problem hiding this comment.
Demandee/demandor?
Source/Consumer?
There was a problem hiding this comment.
The fact that we need to have this discussion probably means the names aren't suitable and are likely to cause some confusion.
I can try using producerDemand and consumerDemand names.
Also you have used "Next" in at least once circumstance
This is a pre-existing method on the DemandChain interface. But it would be a little confusing to rename this to setProducerDemand() when there is no concept of consumer demand on the interface.
| // Fulfill the incoming demand to receive a new frame. | ||
| _upstreamDemand = false; |
There was a problem hiding this comment.
Oh the layer above is called upstream? Hmmmm is the comment to demand wrong?
closes #13043
TransformingFlushertoWebSocketFlusherand update it to use anOutgoingEntryAPI instead offrame + callback + batch.onCompleteFailureonWebSocketFlusherWebSocketFlushernow wraps the callback needed to continue processing in theOutgoingEntryitself.DemandingFlushertoWebSocketDemander, and use a lock instead of multiple atomic variables for the implementation.