-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support to 4.2 by implementing the extended handshake #798
Conversation
driver/src/main/java/org/neo4j/driver/internal/messaging/BoltProtocolVersion.java
Outdated
Show resolved
Hide resolved
driver/src/main/java/org/neo4j/driver/internal/messaging/BoltProtocolVersion.java
Outdated
Show resolved
Hide resolved
@@ -42,9 +42,9 @@ | |||
|
|||
private static final ByteBuf HANDSHAKE_BUF = unreleasableBuffer( copyInt( | |||
BOLT_MAGIC_PREAMBLE, | |||
BoltProtocolV43.VERSION.toInt(), | |||
BoltProtocolV42.VERSION.toInt(), | |||
BoltProtocolV43.VERSION.toIntRange( BoltProtocolV42.VERSION ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering if we should push the range option to the bottom for 4.3 -> 4.2. So the handshake is 4.3, 4.2, 4.1-> 4.0, 3. When 4.3 driver talks to 4.2 server it would skip over 4.2 and negotiate 4.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we will lose the multi database support on the version 4.0, since i will negotiate as 3.5.
Uses 4.1 to talk with a 4.2.x server will not represent any feature drop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point. I guess we just have to be a little careful whilst we switch to the new handshake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a comment on the protocol definition document, to fix the example to consider the version range in decreasing order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that we should go to BoltProtocolV43.VERSION.toIntRange(BoltProtocolVersionV40.VERSION)? That way we are saying all version 4 protocols are accepted by this driver and save 2 slots in the handshake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be done in the protocol, but I think we should keep using the slots for the 4.0 and 4.1, just in case we connect to a non-update server which do not know how to interpret the extend handshake.
} | ||
else if (minorVersion < minVersion.minorVersion) | ||
{ | ||
throw new IllegalArgumentException("Max version should be newest than min version"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know its less consistently applied in the driver but can you format the changes using the server checkstyles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've did a format on the file, what's off in the current format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the space before and after the bracket : throw new IllegalArgumentException( "Max version should be newest than min version" );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐒
Co-authored-by: Florent Biville <445792+fbiville@users.noreply.github.com>
6311523
to
859a784
Compare
The Bolt handshake is a short exchange that occurs after the opening of every Bolt connection. It is used to establish a protocol version for subsequent messaging activity. The sequence is currently limited to four fixed protocol versions; these changes implement a mechanism to extend that to protocol version ranges.
In Bolt v1, the four bytes were treated as a single big-endian unsigned 32-bit integer, with the protocol versioned only via a single number. In Bolt 4.0, the scheme evolved such that bytes D and C represented the major and minor version numbers respectively; bytes A and B were ignored, reserved for future use.
The scheme is to be extended such that byte B can be used to allow the sequence to encode an inclusive range of versions in the client-to-server request. The server-to-client response would remain the same, specifying only a single version.
Specifically, the range minimum and maximum can be computed as follows:
Range maximum = D.C
Range minimum = D.(C - B)
This denotes byte B as a minor version difference, backwards from the minor version in byte C. In other words, zero denotes "zero versions back", one denotes "one version back", and so on. Note that this specifically requires the range to exist within the same major version, but does provide backwards compatibility with existing implementations, which fill bytes A and B with zeroes.
Note also that a range with an identical min and max should be considered semantically equivalent to a single version.