Skip to content

Conversation

@lutovich
Copy link
Contributor

@lutovich lutovich commented Oct 30, 2017

PR consists of two commits:

Separate connections for read/write access modes
NetworkSession acquires connections from the connection pool for the given access mode. It can reuse an existing connection, when available. Previously session did not pay attention to access mode when reusing connections. This could result in READ connection being used for WRITE operation and vice versa.

This commit fixes the problem by making session hold at most two connections at the same time -
one read connection and one write connection. It will now continue using existing read/write connection for read/write operation when available.

Session#reset() releases connection last
Previously it tried to first release connection and them mark existing transaction as terminated. This could've been problematic because it allowed transaction to be used on the client after RESET, which clears connection state on the server.

This commit makes Session#reset() first mark existing transaction as terminated and only then release the connection. Releasing connection implies RESET.

`NetworkSession` acquires connections from the connection pool for the
given access mode. It can reuse an existing connection, when available.
Previously session did not pay attention to access mode when reusing
connections. This could result in READ connection being used for WRITE
operation and vice versa.

This commit fixes the problem by making session hold at most two
connections at the same time - one read connection and one write
connection. It will now continue using existing read/write connection
for read/write operation when available.
Previously it tried to first release connection and them mark existing
transaction as terminated. This could've been problematic because it
allowed transaction to be used on the client after RESET, which clears
connection state on the server.

This commit makes `Session#reset()` first mark existing transaction as
terminated and only then release the connection. Releasing connection
implies RESET.
Copy link
Contributor

@zhenlineo zhenlineo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be easy to add test for:

  • shouldNotUseWriteConnectionForReadAccessMode
    I assume we should not use write conn for read mode as it compromise load-balancing. If a server on purpose gives routing table where writer is not listed as reader, then we should not use the writer for reading even if it is legal.

I also not very sure why we would ever have two types of connections in a session
What will happen if I do:

readSession.Run(...)
// without consuming result, as a result the read conn is local
readSession.writeTx(...)

Shouldn't we close the read conn when we get the new write conn? Otherwise if all the above failed, the two errors will only be thrown when we do session.close, right?
Do we get any benefit to keep to two connections in one session?

@lutovich
Copy link
Contributor Author

Closing in favour of #426

@lutovich lutovich closed this Nov 13, 2017
@lutovich lutovich deleted the 1.5-connection-access-mode branch November 13, 2017 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants