-
Notifications
You must be signed in to change notification settings - Fork 475
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
Implement retry for ServerVersion.AutoDetect
in AddMySqlDbContext
for Pomelo
#2386
Conversation
src/Components/Aspire.Pomelo.EntityFrameworkCore.MySql/AspireEFMySqlExtensions.cs
Outdated
Show resolved
Hide resolved
{ | ||
try | ||
{ | ||
serverVersion = ServerVersion.AutoDetect(connectionString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to "double down" on AutoDetect? @lauxjpn indicated it shouldn't be used in production:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that was the recommendation, but from a GitHub code search it looked like most users (unsurprisingly, really) were taking "the easy way out" and using AutoDetect
. (I.e., I saw AutoDetect
much more in those code search results than a hardcoded version).
I'm at a little bit of a loss to really know what the "right" approach is:
- Stop using
AutoDetect
in Aspire at all: users will have to specify the server version or it will fail at startup. - Strongly encourage specification of a
ServerVersion
in documentation and examples (e.g., sample app usesbuilder.AddMySqlDbContext<MyDbContext>("Catalog", settings => settings.ServerVersion = "8.3.0");
) but keep this current fallback code in case it's omitted. - Even more strongly encourage its use by making
ServerVersion
a parameter to.AddMySqlDbContext()
.
I guess this all boils down to: keep the fallback or not. If there is no fallback, users must supply ServerVersion
and we can debate the ergonomics of how best to accomplish that. Even if we keep the fallback, we could still discuss what affordances should be provided to make it as easy as possible to fall into the pit of success. (Adding a parameter to AddMySqlDbContext
probably isn't right, as it makes it much more difficult to use per-environment configuration.)
My gut feeling right now is that even though using AutoDetect is strongly discouraged by Pomelo, it is provided and supported, and most consumers seem to use it. So for reasons of inertia, Aspire should do the same thing and we can discuss how to encourage people not to, or at least provide good examples in the documentation. Thoughts?
The code was not using any of the additional features of Microsoft.Extensions.Resilience.
This improves performance for Polly: https://www.pollydocs.org/advanced/performance.html#use-static-lambdas.
@eerhardt I've triaged this into P4 but could you decide whether it needs to go in after snap? |
src/Components/Aspire.Pomelo.EntityFrameworkCore.MySql/AspireEFMySqlExtensions.cs
Show resolved
Hide resolved
Remove the resilience strategy from DI.
src/Components/Aspire.Pomelo.EntityFrameworkCore.MySql/AspireEFMySqlExtensions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the contribution!
Be aware that if you use Also, generating migrations without a locally available database server does fail by default when using |
Fixes #1866.
Microsoft Reviewers: Open in CodeFlow