belliottsmith commented on code in PR #4936:
URL: https://github.com/apache/cassandra/pull/4936#discussion_r3594710817
##########
src/java/org/apache/cassandra/transport/Dispatcher.java:
##########
@@ -480,11 +474,19 @@ static Message.Response processRequest(Channel channel,
Message.Request request,
void processRequest(Channel channel, Message.Request request,
FlushItemConverter forFlusher, Overload backpressure, RequestTime requestTime)
{
Message.Response response = processRequest(channel, request,
backpressure, requestTime);
- FlushItem<?> toFlush = forFlusher.toFlushItem(channel, request,
response);
+ FlushItem<?> toFlush = forFlusher.toFlushItem(channel, request,
decorateResponse(response, request));
Message.logger.trace("Responding: {}, v={}", response,
request.connection().getVersion());
flush(toFlush);
}
+ private static Message.Response decorateResponse(Message.Response
response, Message.Request request)
+ {
+ assert response != null;
+ response.setStreamId(request.getStreamId());
Review Comment:
The first two have no `streamId`. I suspect that `InitialConnectionHandler`
must have a zero `streamId` as well. I am not sure it is even correct to set it
there - it isn't set currently, have you checked the change is valid? That is,
it's possible the `streamId` relates to work to be done after the connection is
established, and the error message is meant to be at the connection level? I
don't know, but since it is a change in behaviour we should check. Either way,
we close the connection immediately in the first case, so we are not at risk of
this bug, and we probably should close the connection in the second case as
well since it is an unexpected message on startup so the connection is in a
corrupted state. I would prefer to simply do this for these two cases anyway,
since it's safer without extra validation (no possible change to behaviour, and
we won't be at risk of the bug).
That leaves only the `Dispatcher` case, and you're right that we should
address this in a uniform manner. However, this suggests to me that calls to
`FlushItemConverter` should be forced to go through a method that performs this
work for us, since this is the final step before writing the reply to the wire,
so captures all normal replies. Ideally, we would do something similar for the
out-of-band flushes in `InitialConnectionHandler` and `ExceptionHandlers`, so
that there are at most two routes to send a message. This might be annoying to
have the compiler enforce, but I think it would be OK to settle for a uniform
invoker of toFlushItem, especially if we are diligent about only setting
streamId when we know it, so that we will fail if we hit UNSET_STREAM_ID.
Regarding redundancy: I am not fond of redundancy for its own sake - our
goal should always be to make sure the code is well structured so that we must
use it correctly, and so we can have confidence that all edge cases are
correctly handled (and future edge cases should not be easily added).
Redundancy of the kind discussed here implies we aren't confident we can do
that, which suggests we need to improve our abstractions instead.
This particular redundancy also has downsides, as it has lead to the use of
NO_REQUEST_STREAM_ID in one place where it seems to weaken the protection we
are introducing with UNSET_STREAM_ID, because we're enforcing that it be
supplied in at least one place where it doesn't seem to make semantic sense.
That said, I won't push hard on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
