Conversation
ellbosch
requested review from
chlafreniere,
corivera,
kevcunnane and
udeeshagautam
| # retry logic in case db create fails | ||
| while count < 5: | ||
| for _, _, _, _, is_create_error in client.execute_query(query_db_create): | ||
| if not is_create_error: |
There was a problem hiding this comment.
what if it is a create error? can you try cleaning up? i'm curious what happens if a database just takes longer to create than whatever timeout is set.
in addition, when cleaning up, you probably need to cleanup all test database names, instead of just the last one, if i'm understanding this code correctly.
There was a problem hiding this comment.
that is a good point. Not sure if there is a separate test tear up which should do the "all" cleanup though...
@ellbosch one more small thing - should we try to check for online as well as we discussed to be sure it is "select count(1) from sys.databases where state = 0 and name = '...'
There was a problem hiding this comment.
@ellbosch one more small thing - should we try to check for online as well as we discussed to be sure it is "select count(1) from sys.databases where state = 0 and name = '...'
I was unsuccessful at checking state because the database wasn't appearing in sys.databases. It's a weird issue. It might be related to the fact that we're not logged in to the master database when the create db function is called. I'm really unsure, but I hope that my approach will still be sufficient.
There was a problem hiding this comment.
i'm curious what happens if a database just takes longer to create than whatever timeout is set.
This is a very good point, which I had to go look up and see if this is possible. From what I can deduce, there is no such query that will return before its execution is complete (as seen here). Therefore, I will need to update the PR so that query execution is reattempted.
in addition, when cleaning up, you probably need to cleanup all test database names, instead of just the last one, if i'm understanding this code correctly.
Because this code is only in charge of creating a singular testing db, we just want to delete that one only.
There was a problem hiding this comment.
Correction on my previous comment. The query execution actually does reattempt on each failure. After 5 failures, the method raises an assertion error and a cleanup method gets called on the DB. Given this I believe this PR is good as is, but please feel free to provide more feedback should I be missing something.
There was a problem hiding this comment.
oh duh ok it's the same database name every time. my bad!
| # retry logic in case db create fails | ||
| while count < 5: | ||
| for _, _, _, _, is_create_error in client.execute_query(query_db_create): | ||
| if not is_create_error: |
There was a problem hiding this comment.
how about this... if it is a create error, can we log the error message, so we know what's gone wrong?
There was a problem hiding this comment.
Like this. I'll update.
chlafreniere
left a comment
chlafreniere
left a comment
There was a problem hiding this comment.
One comment around logging error messages. Interested to see how often this retry logic needs to be used in test runs.
|
Latest commit will log a warning whenever a DB create execution fails. I also modified the error message at the end to no longer print values since they were all set to |
3 participants
Fixes #280.
I added retry logic to the create db method used for our tests. In the past this method would occasionally fail due to network issues, so I'm hoping that the retry logic will fix this issue.