Creating a round robin MySQL connection manager

Question

Background

I am working on an e-commerce website, my web server is written in ASP.NET MVC (C#, EF6) and I am using 3 instances of MySQL DB. These DBs are in a cluster (Galera Cluster) and they are automatically replicated.

The problem that I want to solve is load balancing the requests among these 3 instances of MySQL... according to MySQL documentation this can be easily achieved by using a multi-host connectionString, like:

Server=host1, host2, host3; Database=myDataBase; Uid=myUsername; Pwd=myPassword; 

Unfortunately this does not work for Connector/NET (there are multiple open bugs: 81650, 88962, more info here)

My solution

I have tried to implement a simple round robin load balancer for my application.

The aim is to split the multi-host connectionString into multiple single-host connectionStrings, for example the above connectionString would be converted to:

Server=host1; Database=myDataBase; Uid=myUsername; Pwd=myPassword; Server=host2; Database=myDataBase; Uid=myUsername; Pwd=myPassword; Server=host3; Database=myDataBase; Uid=myUsername; Pwd=myPassword; 

The program would use a round robin algorithm to pick one of these connectionStrings. If one of the connections fails, I would ignore that connection for 5 mins and then it would be retried again.

The code

MySQLConnectionManager.cs is going to split each of the multi-host connectionStrings into several single-host connectionStrings, note that I have 2 connectionStrings, one for DataDb and one for LogDb:

<add name="DataDb" connectionString="Server=1.2.3.10, 1.2.3.11; Database=db1; Uid=root; Pwd=..." providerName="MySql.Data.MySqlClient"/> <add name="LogDb" connectionString="Server=1.2.3.10, 1.2.3.11; Database=db2; Uid=root; Pwd=..." providerName="MySql.Data.MySqlClient"/> 

It also has the LoadBalancer instance for each of of the connectionStrings.

MySQLConnectionManager.cs

public static class MySQLConnectionManager { private const string _server = "server"; // in this case, I want one load balancer for DataDb and one for LogDb private static Dictionary<string, DbLoadBalancer> _dbLoadBalancers = new Dictionary<string, DbLoadBalancer>(); static MySQLConnectionManager() { foreach (ConnectionStringSettings multiHostConnectionString in ConfigurationManager.ConnectionStrings) { DbConnectionStringBuilder dbConnectionStringBuilder = new DbConnectionStringBuilder(); var singleHostConnectionStrings = SplitMultiHostConnectionString(multiHostConnectionString.ConnectionString); if (singleHostConnectionStrings.Count > 0) { _dbLoadBalancers.Add(multiHostConnectionString.Name, new DbLoadBalancer(singleHostConnectionStrings)); } } } public static DbConnection GetDbConnection(string connectionStringName) { if (_dbLoadBalancers.ContainsKey(connectionStringName)) { return _dbLoadBalancers[connectionStringName].GetConnection(); } throw new Exception($"Could not find any `connectionString` with name = {connectionStringName}"); } private static List<string> SplitMultiHostConnectionString(string connectionString) { List<string> singleHostConnectionString = new List<string>(); DbConnectionStringBuilder dbConnectionStringBuilder = new DbConnectionStringBuilder() { ConnectionString = connectionString }; if (dbConnectionStringBuilder.ContainsKey(_server)) { string allServers = dbConnectionStringBuilder[_server] as string; string[] allServersArray = allServers.Split(','); foreach (string server in allServersArray) { DbConnectionStringBuilder builder = new DbConnectionStringBuilder(); builder.ConnectionString = dbConnectionStringBuilder.ConnectionString; builder[_server] = server.Trim(); singleHostConnectionString.Add(builder.ConnectionString); } } return singleHostConnectionString; } } 

LoadBalancedConnectionString.cs

public class LoadBalancedConnectionString { public LoadBalancedConnectionString(string connectionString) { ConnectionString = connectionString; LastTimeConnectionWasUsed = DateTime.Now; IsDbAlive = true; } public string ConnectionString { get; set; } public DateTime LastTimeConnectionWasUsed { get; set; } public bool IsDbAlive { get; set; } } 

DbLoadBalancer.cs

public class DbLoadBalancer { private List<LoadBalancedConnectionString> _loadBalancedConnectionStrings; private int _timeToIgnoreFailedDbInMin = 5; private LoadBalancerLogger _logger = null; public DbLoadBalancer(List<string> connectionStrings, string logPath = "") { _loadBalancedConnectionStrings = new List<LoadBalancedConnectionString>(); if (string.IsNullOrEmpty(logPath)) { _logger = new LoadBalancerLogger(logPath); } foreach (string connectionString in connectionStrings) { _loadBalancedConnectionStrings.Add(new LoadBalancedConnectionString(connectionString)); } } public DbConnection GetConnection() { LoadBalancedConnectionString lastUsedConnectionString = null; string message = string.Empty; string lastException = string.Empty; while (_loadBalancedConnectionStrings.Any(c => c.IsDbAlive == true)) { try { lastUsedConnectionString = _loadBalancedConnectionStrings.OrderBy(c => c.LastTimeConnectionWasUsed).FirstOrDefault(); MySqlConnection mySqlConnection = new MySqlConnection(lastUsedConnectionString.ConnectionString); mySqlConnection.Open(); lastUsedConnectionString.LastTimeConnectionWasUsed = DateTime.Now; lastUsedConnectionString.IsDbAlive = true; _logger.Write($"{DateTime.Now}: using: {lastUsedConnectionString.ConnectionString}"); return mySqlConnection; } catch (Exception ex) { lastUsedConnectionString.LastTimeConnectionWasUsed = DateTime.Now.AddMinutes(_timeToIgnoreFailedDbInMin); // don't use this connection for the next 5 mins lastUsedConnectionString.IsDbAlive = false; message += $"{DateTime.Now}: Failed to connect to DB using: {lastUsedConnectionString.ConnectionString}\n"; lastException = ex.Message; _logger.Write($"{DateTime.Now.ToString()}: Fail to connect to: {lastUsedConnectionString.ConnectionString}, marking this connection dead for {_timeToIgnoreFailedDbInMin} min"); } } _logger.Write($"{DateTime.Now}: All connections are dead."); throw new Exception($"Cannot connect to any of te DB instances. {message}\n Exception: {lastException}"); } } 

This is working as expected...

I am using ASP.NET Identity and Ninject for DI, I had to do the following modification to ApplicationDbContext:

[DbConfigurationType(typeof(MySqlEFConfiguration))] public class ApplicationDbContext : IdentityDbContext<ApplicationUser> { // this is how the constructor used to be // public ApplicationDbContext() : base("DataDb") // { // } public ApplicationDbContext() : base(MySQLConnectionManager.GetDbConnection("DataDb"), true/*contextOwnsConnection*/) { } public static ApplicationDbContext Create() { return new ApplicationDbContext(); } protected override void OnModelCreating(DbModelBuilder modelBuilder) { base.OnModelCreating(modelBuilder); } } 

As you can see I am passing true for contextOwnsConnection, I believe (for EF6) this means the connection should be terminated once DbContext is disposed... this seems the logical choice for me, but would be great to get some feedback on this... I am worried to leave open connections around... or close the connections to soon (so far it has passed my initial tests)

And this is the Ninject code, since this is a web project, I am using InRequestScope() for all dependencies.

private static void RegisterServices(IKernel kernel) { kernel.Bind<ApplicationDbContext>().ToSelf().InRequestScope(); kernel.Bind<ICategoryRepository>().To<CategoryRepository>().InRequestScope(); ... } 

---

Before trying to reinvent the wheel, I contacted Oracle and pointed out that this issue has been open since 2016... then I tried to use AWS Network Load Balancer but I did not manage to get it working see here... on the Galera documentation, I see many solutions use HAProxy, but this means I would have to set up 2 new servers (to have a fault tolerant HAProxy)... which seems too much investment in hardware... I also looked into using a DNS record with multiple IPs to split the load among different DBs... this apraoch seems a bit controversial... also I was also not sure if AWS CloudFront caching would cause any issues, so decided not to use it.

Note: I have put the final code in Github, in case anyone wants to use it.

---

Update

This issue has been fixed in Connector/NET 8.0.19, see: https://insidemysql.com/

Answer 1

Aliveness issue

As it is, your code does not perform the task you expect it to, because the LoadBalancer never reinstates "dead" connection pools. Notice that you only ever set LoadBalancedString.IsDbAlive to true in the constructor or if another connection is alive after the timeout expired.

If ever all the servers managed in the DbLoadBalancer are offline, the balancer never recovers.

This can be remediated by adjusting the lookup you perform. Instead of using a while-loop that is never executed if all connections are dead, you may want to iterate over all connection strings regardless of their IsDbAlive status:

public DbConnection GetConnection() { foreach (var candidate in _loadBalancedConnectionStrings.OrderBy(c => c.LastTimeConnectionWasUsed)) { if (!candidate.IsDbAlive && DateTime.Compare(c.LastTimeConnectionWasUsed, DateTime.Now) > 0) { // connection is not alive, no retry yet -> skip connection continue; } // try-catch goes here } } 

Sidenote: candidate here takes the place of lastUsedConnectionString. I like my name better ;)

Typing Effort

I notice that for the vast majority of your variable declarations you explicitly declare the type of the variable. For quite a while now C# supports strictly typed left-hand side type inference (implemented with the var keyword).

It's idiomatic (and common) for C# code to use var on the left hand side for all declarations that allow it.

Answer 2

MySQLConnectionManager

DbConnection GetDbConnection() and List<string> SplitMultiHostConnectionString()

Whenever you also need the value for a given key of a Dictionary<TKey, TValue> you shouldn't useContainsKey() together with the Item getter but you should use TryGetValue().

Internally these three methods are calling the FindEntry() method to check wether a given key exists. So calling this method only once through the TryGetValue() method should be the way to go.

DbLoadBalancer

DbConnection GetConnection()

Don't compare a bool against true or false just use the
bool as the condition.

Don't use

while (_loadBalancedConnectionStrings.Any(c => c.IsDbAlive == true)) 

but

while (_loadBalancedConnectionStrings.Any(c => c.IsDbAlive)) 

Because you don't change neither _loadBalancedConnectionStrings nor _logger you should make these variables readonly to prevent accidentally changes.

I think for the constructor you didn't want

if (string.IsNullOrEmpty(logPath)) { _logger = new LoadBalancerLogger(logPath); } 

but something different. I can't tell excatly what because it wouldn't make sense either to revert the condition. Right now if you pass a value other than "" any method call of _logger will result in a NullReferenceException. By reverting the condition (which would make more sense) but passing "" would result in the same exception.

Passing a List<string> connectionStrings to the constructor restricts you to only use a List<string>. Hence if you e.g have string[] or an IEnumerable<string> you would need to change this into a List<string> but because you are only enumerating the items you should better use just an IEnumerable<string>.

---

General

• Declare variables as near to their usage as possible.
• Your naming is good. Especially you name your things in respect to the .NET Naming Guidlines.

Answer 3

You can improve the testability of your code by:

• Refactoring SplitMultiHostConnectionString() into it's own class
• Passing in the connection strings instead of accessing ConfigurationManager (thus removing the dependency on ConfigurationManager)
• This will make your class no longer static, but you can make it a singleton via the DI framework
• moving the logic around mySqlConnection.Open(); into it's own function, since that call makes the entire thing untestable

Quality issues:

• Do not throw or catch System.Exception. In your case in particular, you need to be able to distinguish if any error is retryable. For example, you'll fail ungracefully if the connection string is malformed. Better: Review what exceptions are going to be thrown by the called code.
• Code is not thread safe. You also update LastTimeConnectionWasUsed only after opening the connection, giving other threads an large window to use that same connection. Better: Use ConcurrentQueue
• I would not set LastTimeConnectionWasUsed five minutes into the future. This will make the code harder to maintain and debug. Better: Put broken connections into a standby list instead of implicitly bringing them back into rotation.

I think it's a tricky thing to get 100% right. I'd load balance with an external load balancer like nginx.