Jobs.Common helpers for NuGet.Services.Sql #505
Conversation
| return c; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Dead code, since NuGet.Services.Sql was introduced
src/NuGet.Jobs.Common/JobBase.cs
Outdated
|
|
||
| internal static string GetDatabaseKey<T>() | ||
| { | ||
| return typeof(T).Name; |
There was a problem hiding this comment.
@loic-sharma @agr this was the cause for my earlier regression, and why I needed to revert. I meant to cache on IDbConfiguration type name. Instead, I cached on "T" and only the last db registration persisted.
Fixed and added test.
chenriksson
requested review from
joelverhagen,
agr,
loic-sharma,
shishirx34,
ryuyu,
cristinamanum,
scottbommarito and
skofman1
| /// <summary> | ||
| /// Test connection early to fail fast, and log connection diagnostics. | ||
| /// </summary> | ||
| private async Task TestConnection(string name, ISqlConnectionFactory connectionFactory) |
There was a problem hiding this comment.
I don't understand how this "fails fast". Isn't the failure swallowed into a log?
There was a problem hiding this comment.
Yes, "warn fast" is more correct - just wanted a way to verify on startup whether the DB connections are configured correctly.
Maybe I should retry/throw in this case?
There was a problem hiding this comment.
Updated to really fail fast. Will retry on job restart, and hopefully we can add heartbeat in future to detect startup failures.
chenriksson
force-pushed
the
chenriks-jobscommon-sql
branch
from
d41763d to
10561f4
Compare
chenriksson
changed the title
Work towards stabilization of SQL AAD connections for jobs
This is stripped down version of original PR: #485
NuGet.Services.SqlNext steps: