[RESOLVED] How to make this Database Fail Over System Code better?

[mnxford]

Hello,

Actually I have coded a Database Fail Over System which check the connection with one database and if the connection fails it switches to the backup database automatically and also it has the feature to check from the main database a value to set the user's current database as primary or secondary. The Code:

Public Sub DBFailOver()
Try
Dim dbprimary As String
Dim Port As Integer = 3306
Dim Client As TcpClient = Nothing
Dim SubClient1 As TcpClient = Nothing
Dim SubClient2 As TcpClient = Nothing
Dim SubClient3 As TcpClient = Nothing
Dim SubClient4 As TcpClient = Nothing
Dim SubClient5 As TcpClient = Nothing
Try
Client = New TcpClient(MainDBHostname, Port)
Using command As New MySqlCommand("My Query", con)
command.Parameters.AddWithValue("", 1)
con.Open()
Using reader = command.ExecuteReader()
With reader.Read()
dbprimary = reader("...").ToString
End With
End Using
End Using
con.Close()
If CDbl(dbprimary) = 1 Then
Try
SubClient1 = New TcpClient(MainDBHostname, Port)
link = My.Settings.MainDB.ToString
Catch ex1 As SocketException
Try
SubClient2 = New TcpClient(BackupDBHostname, Port)
link = My.Settings.BackupDB.ToString
Catch ex2 As SocketException
Finally
If Not SubClient2 Is Nothing Then
SubClient2.Close()
End If
End Try
Finally
If Not SubClient1 Is Nothing Then
SubClient1.Close()
End If
End Try
ElseIf CDbl(dbprimary) = 0 Then
Try
SubClient3 = New TcpClient(BackupDBHostname, Port)
link = My.Settings.BackupDB.ToString
Catch ex3 As SocketException
Try
SubClient4 = New TcpClient(MainDBHostname, Port)
link = My.Settings.MainDB.ToString
Catch ex4 As SocketException
Finally
If Not SubClient4 Is Nothing Then
SubClient4.Close()
End If
End Try
Finally
If Not SubClient3 Is Nothing Then
SubClient3.Close()
End If
End Try
End If
Catch ex As SocketException
Try
SubClient5 = New TcpClient(BackupDBHostname, Port)
link = My.Settings.BackupDB.ToString
Catch ex5 As SocketException
Finally
If Not SubClient5 Is Nothing Then
SubClient5.Close()
End If
End Try
Finally
If Not Client Is Nothing Then
Client.Close()
End If
End Try
Catch ex As Exception
MsgBox(ex.ToString)
Application.Exit()
End Try
End Sub

So, I was wondering is there any chance I can make this code more optimized and better? Guys please provide your suggestion on improving this code.


Thanks in advance...

[mnxford]

Bump

[mnxford]

Anyone to help in this?

[mnxford]

Sitten Spynne Can you give your suggestion on the above code?

[Sitten Spynne]

How is it that you managed to use Code tags but remove all formatting? It makes it really difficult to read.

Politely: no, this is not very good at all. It's a lot of copy/pasted code and has so many levels of nesting it's hard to understand where we are. Everything that looks the same between each Try/Catch is something that could be accomplished either with a loop or helper methods or both.

I don't really understand a lot about the code, but if I had to write a system that tries several different connections, it would look like this:

Public Function ConnectWithFailover(ByVal validAddresses() As String) As TcpClient
  For Each address In validAddresses
    Try
      Return CreateClient(address)
    Catch ex As SocketException
      ' Log that a connection failed, then continue.
    End Try
  Next

  Throw New ApplicationException("None of the listed servers responded.")
End Function

Private Function TryCreateClient(address as String) As TcpClient
  Return New TcpClient(address, _port)
End Function

This tries each address, and returns a TcpClient if one successfully connects. If not, it throws an exception.

Your code looks like it could similarly make use of loops and helper methods, but without indentation I don't have a chance of deciphering what it's trying to do. I think it'd be easier if you describe what your failover process is in detail and let us try it from scratch than it would be to try and figure it out from this code, even with indentation. I got the feeling the level of indentation was something like 6, and usually when you get past 3 it's a warning sign.

(Also it's sort of considered impolite to bump topics, and it's only by luck that I stumbled across this thread. If you want someone to poke their head in, you an send a PM, but it's not a guarantee and maybe some people consider it rude? I don't know. There's some people here that get upset when you suggest they help people.)

[mnxford]

Actually I am really sorry for the formatting mistake I have done. I am really scratching my head now thinking about how I could possibly paste this non-formatted code here. The real actual code is:

Public Sub DBFailOver()
        Try
            Dim dbprimary As String
            Dim Port As Integer = 3306
            Dim Client As TcpClient = Nothing
            Dim SubClient1 As TcpClient = Nothing
            Dim SubClient2 As TcpClient = Nothing
            Dim SubClient3 As TcpClient = Nothing
            Dim SubClient4 As TcpClient = Nothing
            Dim SubClient5 As TcpClient = Nothing
            Try
                Client = New TcpClient(MainDBHostname, Port)
                Using command As New MySqlCommand("my query for selecting which databse to use now", con)
                    command.Parameters.AddWithValue("@", 1)
                    con.Open()
                    Using reader = command.ExecuteReader()
                        With reader.Read()
                            dbprimary = reader("db-primary").ToString
                        End With
                    End Using
                End Using
                con.Close()
                If CDbl(dbprimary) = 1 Then
                    Try
                        SubClient1 = New TcpClient(MainDBHostname, Port)
                        link = My.Settings.MainDB.ToString
                    Catch ex1 As SocketException
                        Try
                            SubClient2 = New TcpClient(BackupDBHostname, Port)
                            link = My.Settings.BackupDB.ToString
                        Catch ex2 As SocketException
                        Finally
                            If Not SubClient2 Is Nothing Then
                                SubClient2.Close()
                            End If
                        End Try
                    Finally
                        If Not SubClient1 Is Nothing Then
                            SubClient1.Close()
                        End If
                    End Try
                ElseIf CDbl(dbprimary) = 0 Then
                    Try
                        SubClient3 = New TcpClient(BackupDBHostname, Port)
                        link = My.Settings.BackupDB.ToString
                    Catch ex3 As SocketException
                        Try
                            SubClient4 = New TcpClient(MainDBHostname, Port)
                            link = My.Settings.MainDB.ToString
                        Catch ex4 As SocketException
                        Finally
                            If Not SubClient4 Is Nothing Then
                                SubClient4.Close()
                            End If
                        End Try
                    Finally
                        If Not SubClient3 Is Nothing Then
                            SubClient3.Close()
                        End If
                    End Try
                End If
            Catch ex As SocketException
                Try
                    SubClient5 = New TcpClient(BackupDBHostname, Port)
                    link = My.Settings.BackupDB.ToString
                Catch ex5 As SocketException
                Finally
                    If Not SubClient5 Is Nothing Then
                        SubClient5.Close()
                    End If
                End Try
            Finally
                If Not Client Is Nothing Then
                    Client.Close()
                End If
            End Try
        Catch ex As Exception
            Splash.Splash_Timer.Stop()
            WriteLog("ERROR #121: Problem with Database Fail Over System!!! Please report the error number to the developer.", True)
            WriteLog(ex.ToString, False)
            WriteLog("------------------------------------------------------------------------------------", False)
            MsgBox("ERROR #121: Problem with Database Fail Over System!!! Please report the error number to the developer.", MsgBoxStyle.Critical, "ERROR")
            Application.Exit()
        End Try
    End Sub

Now let me describe the whole process done by this Fail Over system. First of all it checks if primary database hostname is alive using TcpClient in my MySQL Server port. If primary server didn't respond then it will check for a respond from the backup database server using the same way. If backup also doesn't response then it triggers an error msg. Secondly If the primary database connection was found then it will check my database for information on which database between primary and secondary would serve the software. If primary database was selected after the database query then it will again check for if primary database hostname available on port 3306 which is my MySQL server port. If no respond from the primary hostname then it will again switch over to check secondary hostname. If secondary database is also not reachable then an exception occurs. All the code is just a fail over system which will return a different value for the connection string in different circumstances.

Thanks in advance...

[Sitten Spynne]

Hmm. OK. So it sounds like you have 2 servers, and 2 different processes.

The first process is asking the question, "Which database should I be using?" Either server can answer this question, and if one's unreachable you want to ask the other.

The second process is asking the question, "Can I use the database that I'm supposed to use?".

So you could connect to the Main database, then be told the Secondary is "right", but not be able to reach the Secondary and fail. Or try connecting to Main, fail, then connect to Secondary and be told Main is right, then you fail.

I don't think you need TcpClient at all. I'm headed for bed soon and I'm not too familiar with these APIs but this is how I think I'd structure it, I'm sure someone will come in after me and fill in the blanks:

Dim databaseInfo() As DatabaseInfo = ' Some custom types that represent each database, including the connection string for that database.

Dim dbPrimary As Double = -1
For Each dbInfo in databaseInfo
  Try
    ' If a previous attempt found a primary, look for the DB with
    ' that ID.
    If dbInfo.Id = dbPrimary Then
      ' We did it! We found the database we should try. Maybe it doesn't
      ' work. We'll find that out later, or can try to connect to it
      ' later.
      Return ???
    End If

    ' We haven't found a matching dbPrimary yet, try connecting to this database
    ' to find one.
    Using connection = new WhateverConnection(dbInfo.ConnectionString)
      connection.Open()
      ' This function does the work to ask which DB should be primary, you can imagine what it does.
      dbPrimary = GetPrimaryDb(connection)
      If dbPrimary = dbInfo.Id Then
        ' We win, this dbInfo is the one we should use!
        Return ???
      End If
    End Using
  Catch ex As WhateverException
    ' Well, shoot. Can't even connect to this database.
    ' I like to log this stuff, but whatever floats your boat.
  End Try
Next

' If we get here, we didn't return a winner, so no databases were sufficient.

There's a lot of ways to improve that with helper methods, and a lot of blanks to fill. But I hope it inspires you. The idea here is pinging the server may not mean we can connect to it, so we just try. If we can connect, we ask what the primary DB should be. If we manage to find a DB with that ID in our list, we return it.

Now, that doesn't mean we know we can connect to it, but it sort of doesn't matter. We asked someone what DB we should use and got an answer. If, later, we try to connect to it and that fails, we can die there. If you did a little extra work, you could test it here, but I think it's a little frivolous. It's sort of like checking if a file exists: it can still get deleted after you ask, so the best way to find out if it's there is try to use it and catch FileNotFoundException.

Anyway, say we fail the first DB connection. We try the next. And we keep going until someone tells us what our primary should be. If we don't manage to connect to any databases, or we failed the primary but connected to some other server that then told us a primary, we end up at the end of the method and can detect this state.

[mnxford]

Sitten Spynne really thanks for your suggestion. I got a very good picture of the Fail Over system I should use from your suggestion. I will try to get it working today. Since then if anyone wants to suggest any other thing then your most welcome.