Byte arrays support #370
Byte arrays support #370
Conversation
…tocol. Background: A connection holds a socketClient. A socketClient has a protocol which specifies what protocol version the driver should work on. When a connection is created in a pool, the connection will first be `started` (where the socketClient will negotiate protocol version with the server) and then `initialized` (where the server version will be sent back). Problem description: The problem that we need to solve here is that for 3.2+ servers, the protocol the driver is working on should allow bytes, while for server version lower than 3.2.0, the protocol should disallow bytes to be sent to prevent from crashing server connection in a bad way. While the server version is only known after the protocol is created, so after the server version arrived the protocol need either be updated (this commit) or the protocol need to be informed (previous impl). Solution: This commit changed the behaviour in connection to update protocol regarding bytes support right after server version is received. By doing in this way we avoid some two-direction reference between connection and protocol.
There was a problem hiding this comment.
@zhenlineo review round completed. Changes generally look good. I think we should use consistent naming - "byte array" instead of "bytes". There is also a bit of code duplication.
PR should go in 1.4 when we create such branch.
| @@ -167,6 +168,7 @@ public synchronized void flush() | |||
| } | |||
| catch ( IOException e ) | |||
| { | |||
| close(); | |||
There was a problem hiding this comment.
Why do we need to close here?
I think connection will be properly close by the session if it throws from here.
|
|
||
| ChunkedOutput output = new ChunkedOutput( channel ); | ||
| BufferingChunkedInput input = new BufferingChunkedInput( channel ); | ||
| public static SocketProtocol create( ByteChannel channel, boolean byteArraySupportEnabled ) |
There was a problem hiding this comment.
Could this method be named createWithoutByteArraySupport or simply withoutByteArraySupport? We could then get rid of second boolean parameter.
|
|
||
| this.writer = new PackStreamMessageFormatV1.Writer( output, output.messageBoundaryHook() ); | ||
| this.reader = new PackStreamMessageFormatV1.Reader( input, input.messageBoundaryHook() ); | ||
| public SocketProtocolV1( ByteChannel channel, boolean byteArraySupportEnabled ) |
There was a problem hiding this comment.
Constructor can be private
| @@ -54,6 +54,12 @@ public static ServerVersion version( Driver driver ) | |||
| } | |||
| } | |||
|
|
|||
| public static ServerVersion version( Session session ) | |||
There was a problem hiding this comment.
I think it is better to inline this method in ParametersIT because it is only used there
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public class BytesValue extends ValueAdapter |
There was a problem hiding this comment.
Should we maybe also implement asList() and asList(Function<Value,T>) methods?
| @@ -114,6 +116,11 @@ public static Value value( Value... input ) | |||
| return new ListValue( values ); | |||
| } | |||
|
|
|||
| private static Value bytesValue( byte[] input ) | |||
| @@ -107,7 +107,7 @@ public PackOutput writeDouble( double value ) throws IOException | |||
| return this; | |||
| } | |||
|
|
|||
| private void ensure( int size ) throws IOException | |||
| private void ensure(int size ) throws IOException | |||
| @@ -501,6 +501,29 @@ public double unpackDouble() throws IOException | |||
| throw new Unexpected( "Expected a double, but got: " + toHexString( markerByte )); | |||
| } | |||
|
|
|||
| public byte[] unpackBytes() throws IOException | |||
There was a problem hiding this comment.
Is this method different from #unpackRawBytes()?
Looks like only error message has changed. Can we remove #unpackRawBytes()?
There was a problem hiding this comment.
@zhenlineo changes look good. PR can be merged after changing base branch to 1.4
No description provided.