New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SqlFilter junction bug and retry attempts in exception #468

Merged
merged 1 commit into from Nov 27, 2018

Conversation

Projects
None yet
2 participants
@jeremeguenther
Collaborator

jeremeguenther commented Nov 27, 2018

  • Fixed a bug where the SqlFilter incorrectly injects a junction value even if the current filter group has just begun.
  • Fixed a couple of visual studio return formattings.
  • Added the ability for the number of retry attempts at calling the db to be recorded in the custom exception.
  • Fixed bug where try/catch for retry code only catches SqlExceptions. Brought it into standard with the code when that feature is turned off by adding a subsequent catch for all Exceptions.
SqlFilter junction bug and retry attempts in exception
- Fixed a bug where the SqlFilter incorrectly injects a junction value even if the current filter group has just begun.
- Fixed a couple of visual studio return formattings.
- Added the ability for the number of retry attempts at calling the db to be recorded in the custom exception.
- Fixed bug where try/catch for retry code only catches SqlExceptions. Brought it into standard with the code when that feature is turned off by adding a subsequent catch for all Exceptions.

@jeremeguenther jeremeguenther requested a review from niemyjski Nov 27, 2018

@niemyjski

LGTM, only question that I can't see from looking at this, will the base exception that's now being caught be logged and rethrown? Either way I'm fine with this.

@jeremeguenther

This comment has been minimized.

Collaborator

jeremeguenther commented Nov 27, 2018

I do not think we have had logging at this particular level. Here is what the method looks like generated with 3 retry attempts and a 500 millisecond retry delay:

        public static IDataReader ExecuteReader(Database database, DbCommand dbCommand)
        {
            
            System.Diagnostics.Stopwatch timer = new System.Diagnostics.Stopwatch();
            timer.Start();
            IDataReader results = null;
            for (int attempt = 1; attempt <= 3; attempt++)
            {
                try
                {
                    results = database.ExecuteReader(dbCommand);
                    break;
                }
                catch (System.Data.SqlClient.SqlException ex)
                {
                    // we could time out due to locks 
                    // or our database may be temporarily unavailable
                    if (attempt == 3)
                    {
                        timer.Stop();
                        throw new NetTiersDataException(string.Format("Reader Error: {0}", dbCommand.CommandText), ex, dbCommand, attempt, timer.ElapsedMilliseconds);
                    }
                    System.Threading.Thread.Sleep(500);
                }
                catch (Exception ex)
                {
                    timer.Stop();
                    throw new NetTiersDataException(string.Format("Reader Error: {0}", dbCommand.CommandText), ex, dbCommand, attempt, timer.ElapsedMilliseconds);
                }
            }

            return results;
        }

@jeremeguenther

This comment has been minimized.

Collaborator

jeremeguenther commented Nov 27, 2018

For clarification. The exceptions in both cases are wrapped and thrown again. So no exception data is being lost.

@jeremeguenther jeremeguenther added the bug label Nov 27, 2018

@jeremeguenther

This comment has been minimized.

Collaborator

jeremeguenther commented Nov 27, 2018

To finalize the pull request, is that something that you as the reviewer do? Or am I supposed to chose one of the three options?
If I am supposed to do it, do I use the default "Create a merge commit", or do I use one of the other two options?

@niemyjski

This comment has been minimized.

Contributor

niemyjski commented Nov 27, 2018

Looks good!, I just have to approve, but if you think it's good just override and approve it :). I usually do squash and merge but it's completely up to you :)

@jeremeguenther

This comment has been minimized.

Collaborator

jeremeguenther commented Nov 27, 2018

Sounds good. Once you have approved something I will squash and merge it. Just wanted to make sure I was doing things the way that is expected for this project.

@jeremeguenther jeremeguenther merged commit 631eb49 into netTiers:master Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment