CASSGO-97 Protocol version negotiation doesn't work if server replies with stream id different than 0 #1920
Conversation
|
WIP: currently working on tests for the whole protocol negotiation logic.... |
…am id different than 0 Previously, protocol negotiation didn't work properly when C* was responding with stream id different from 0. This patch changes the way protocol negotiation works. Instead of parsing a supported protocol version from C* error response, the driver tries to connect with each supported protocol starting from the latest. Patch by Bohdan Siryk; Reviewed by <> for CASSGO-98
|
Ready for review |
| } | ||
| if err == nil { | ||
| c.session.logger.Debug("Discovered protocol version using host.", | ||
| NewLogFieldInt("protocol_version", connCfg.ProtoVersion), NewLogFieldIP("host_addr", host.ConnectAddress())) |
There was a problem hiding this comment.
any particular reason you removed the host_id log field from both calls? Is it because it's not populated?
There was a problem hiding this comment.
Yes, it is not populated, so there is really no useful information
|
|
||
| // if a client uses an unsupported protocol version, we respond with an error | ||
| if !r.supportsProtocol(reqFrame.header.version) { | ||
| respFrame.writeHeader(0, opError, reqFrame.header.stream) |
There was a problem hiding this comment.
can we have a test case that replies with the stream id of the request in addition to this one? This behavior is not defined in the spec and I think some C* versions might behave differently
There was a problem hiding this comment.
Could you please clarify what you exactly mean? The test server already replies with the stream id of the request
There was a problem hiding this comment.
Oh I'm sorry I thought it was replying with stream id 0, in that case then can you add a test case where the response stream id is 0 and the request stream id is not 0?
| c.session.logger.Debug("Discovered protocol version using host after parsing protocol error.", | ||
| NewLogFieldInt("protocol_version", proto), NewLogFieldIP("host_addr", host.ConnectAddress()), NewLogFieldString("host_id", host.HostID())) | ||
| return proto, nil | ||
| c.session.logger.Debug("Failed to connect to the host using protocol version.", |
There was a problem hiding this comment.
We should only attempt to reconnect to the same host with a lower protocol version if the error is assumed to be related to unsupported protocol version:
- t's an error to the first request (OPTIONS)
- the error type is PROTOCOL_ERROR or SERVER_ERROR - SERVER_ERROR is for old C* versions that reported this error as SERVER_ERROR instead of PROTOCOL_ERROR
I propose we use a custom internal error type (unsupportedProtocolVersionErr for example) just for this case that we can return inside the dial method and we test for it here in discoverProtocol(). Reference code in the java driver here.
Technically the java driver does check for the error string which I didn't know but I still believe we shouldn't do that, checking the above 2 conditions is enough.
There was a problem hiding this comment.
That makes sense, we don't really have to re-try each time if the error is not protocol-related
| //go:build all || unit | ||
| // +build all unit | ||
|
|
||
| package gocql |
There was a problem hiding this comment.
add license header, we should add a check for this in CI at some point
| return nil | ||
| } | ||
|
|
||
| func TestProtocolNegotiation(t *testing.T) { |
There was a problem hiding this comment.
Very good job with this test, I feel like it was a weak spot in our test suite and now it's much better. Can you add -v to the go test command in the GH workflow step that runs the unit tests so we can double check which tests are running?
There was a problem hiding this comment.
Thanks. I'll add verbose for unit tests to ci
|
Thanks, @joao-r-reis, for your feedback. I'm going to address this next week |
2 participants
Protocol version negotiation doesn't work if server replies with stream id different than 0
Previously, protocol negotiation didn't work properly when C* was responding with stream id different from 0.
This patch changes the way protocol negotiation works. Instead of parsing a supported protocol version from C* error response, the driver tries to connect with each supported protocol starting from the latest.
Patch by Bohdan Siryk; Reviewed by <> for CASSGO-98