Add support for maintaining different connector lists for different connection string #9
Conversation
|
What happens when the number of unique connection strings are more than the number of connectors (i.e. number of connections in a pool)?
|
Since we call the OpenNewConnector of the Pooling datasource first from inside our OpenNewConnector, it has a check in there for max connections to a pool, if that is false, it just throws an error there itself. |
So this behaviour would be different from that of the upstream driver. |
ashetkar
left a comment
There was a problem hiding this comment.
Could not run the tests on my system due to different reasons related to my env, but the changes look fine with some minor comments.
|
Please hold, I'm now trying these on my dev-server, will update once completed. |
ashetkar
left a comment
There was a problem hiding this comment.
I'm seeing some test failures, please look into those as well.
| { | ||
| if (ex.Message.Equals("No suitable host was found", StringComparison.OrdinalIgnoreCase)) | ||
| Console.WriteLine("Expected Failure:" + ex.Message); | ||
| Console.WriteLine("Caught expected exception:" + e.Message); |
There was a problem hiding this comment.
Why is there no need to check for the exact message string now?
There was a problem hiding this comment.
This was initially just printing errors for NoSuitableHostException, instead of all errors
There was a problem hiding this comment.
So does this mean all NpgsqlExceptions were and are expected?
| { | ||
| if (ex.Message.Equals("No suitable host was found", StringComparison.OrdinalIgnoreCase)) | ||
| Console.WriteLine("Expected Failure:" + ex.Message); | ||
| Console.WriteLine("Caught expected Exception:" + e.Message); |
ashetkar
left a comment
There was a problem hiding this comment.
LGTM, subject to perf numbers being fine
|
Perf tests results:
|
Problem statement:
Since the _pools variable (list of pools. Each pool corresponds to one node) is static, it only has one connectors list per host. These connectors are created with the connection configuration with which each of the pools were created.
In this case, when the connection string changes, the connector created from previous conn string is given if it is idle.
Solution:
Create a wrapper around PoolingDatasource which maintains two new maps, connection string to connectors and connection string to idle connectors map. When an idle connector is available, check the connection string and give a connector corresponding to that. Same with when a connector is to be returned.
Tests:
Added Tests in YBPoolingWrapperTests:
- With pooling, the average for all iterations (minus the first one) is 0ms for both the upstream and our driver
- Without pooling, the average time for all iterations (minus the first one) is 138ms for upstream driver and 147ms for smart driver.
Problem statement 2:
In case of a 6 node cluster where the configuration was :
127.0.0.1 (primary) -> datacenter1
127.0.0.2 (primary), 127.0.0.4(read replica) -> datacenter2
127.0.0.3 (primary), 127.0.0.5(read replica) -> datacenter3
127.0.0.6(read replica) -> datacenter 4
Topology key priority: datacenter2:1, datacenter3:2
In case of prefer primary, when 127.0.0.2 and 127.0.0.3 are down, the connection should go to 127.0.0.1 (last primary node). But instead it was going to 127.0.0.4.
Solution:
This was since prioritytopoolindex was a single map with both primary and read replica nodes. So this created an intermittent issue where for priority 1, it could either store 127.0.0.2 or 127.0.0.4. If it stored 127.0.0.1, when betterNodeAvailable was called, it would see the node is down and move on to the next node and the test would pass, but when 127.0.0.4 was stored, it would see that the node is higher up in priority even though it was a read replica node and the connections would go to that node and fail.
The priority map is separated now to primary and RR.
7f01b63
Source Code Changes
src/Npgsql/ClusterAwareDataSource.cs
src/Npgsql/TopologyAwareDataSource.cs
Test Changes