Skip to content
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

RevEng: Command timeout should be configurable #22301

Closed
AndriySvyryd opened this issue Aug 28, 2020 · 21 comments
Closed

RevEng: Command timeout should be configurable #22301

AndriySvyryd opened this issue Aug 28, 2020 · 21 comments
Labels
area-scaffolding closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported type-enhancement

Comments

@AndriySvyryd
Copy link
Member

See #22287

@svengeance
Copy link
Contributor

I would be interested in taking this if the scope is just a configurable timeout, if that's acceptable.

@AndriySvyryd
Copy link
Member Author

@svengeance Sure, propose how a user would configure it

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 30, 2020

Now, if only it was possible to change the default via the connection string! dotnet/SqlClient#614

@svengeance
Copy link
Contributor

svengeance commented Aug 30, 2020

I outlined this a bit in the issue that this one rose from, but I could implement this in one of a few ways.

The easiest (well. ease-of-use wise) solution would be to introduce a new command line argument specifying a default timeout. This would then be carried forward from dotnet-ef and ef to EFCore.Design, and all the way to DbConnection resolution inside of SqlServerDatabaseModelFactory.
The primary benefit here is that the access point is up-front and easy to configure.

I had in my mind to attempt to bring in the same methodology of resolving DbContexts from IDesignTimeDbContextFactory, but I'm not sure if it feels weird to enact a database-first flow through a code-first design time context.
The primary benefit of this implementation is that it follows the same concept that Migrations. It is, of course, more complicated than a cmdline arg. Users would specify the context type ala migrations.

Lastly, we could potentially extend DatabaseModelFactory to include a timeout property. I need to verify, but I believe that users have the potential to extend this particular design time service through both migrations and scaffolding. Users could then create their own class extending from their provider-specific model factories (e.g. SqlServerDatabaseModelFactory) and override this field, which would give them a method that's not as intuitive, but has less internal moving parts.

Of these 3 proposals, did any of them align with your own ideas? None of them are breaking for existing users, and offer various levels of precedence, internal complexity, and ease-of-use.

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 30, 2020

and all the way to DbConnection resolution inside of SqlServerDatabaseModelFactory.

Keep in mind that the Timeout must be set on each command object.

@smitpatel smitpatel added this to the Backlog milestone Aug 31, 2020
@AndriySvyryd
Copy link
Member Author

Triage: This should be implemented as a new command line argument consistent with the arguments we already have for other commands. @bricelam to provide details.

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 1, 2020

In the meantime, I will try to implement this: dotnet/SqlClient#614

@AndriySvyryd
Copy link
Member Author

@bricelam Ping for command line argument name

@bricelam
Copy link
Contributor

bricelam commented Sep 9, 2020

I'm with @ErikEJ on this one. No point adding an option if you can specify it in the connection string.

@bricelam bricelam removed this from the Backlog milestone Sep 9, 2020
@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 10, 2020

PR well underway

@ajcvickers ajcvickers added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. and removed needs-design labels Sep 11, 2020
@ajcvickers
Copy link
Member

ajcvickers commented Sep 11, 2020

We discussed this and agreed that setting the command timeout in the connection string is the preferred way to add this functionality.

@bgrainger
Copy link
Contributor

@ajcvickers Yes; the option is DefaultCommandTimeout: https://mysqlconnector.net/connection-options/#DefaultCommandTimeout.

@bricelam
Copy link
Contributor

Is there a de-facto keyword name? I was going to use Default Timeout to match System.Data.SQLite, but I'm happy to make that just an alias...

@bricelam
Copy link
Contributor

Looks like Command Timeout is the most common. I'll go with that.

@bgrainger
Copy link
Contributor

MySqlConnector also supports Command Timeout (and Default Command Timeout) as an alias for the connection string option.

@bricelam
Copy link
Contributor

Looks like Oracle has Connect(ion) Timeout, but none for commands.

@bricelam
Copy link
Contributor

@cincuranet Wanna join the party and add a Command Timeout keyword too? 😉

@bricelam
Copy link
Contributor

Looks like IBM has Query Timeout.

@ErikEJ
Copy link
Contributor

ErikEJ commented Sep 12, 2020

@cincuranet This is about Command execution timeout, not about Connection timeout.

@cincuranet
Copy link
Contributor

Thanks @ErikEJ. In that case it's tracked here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-scaffolding closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported type-enhancement
Projects
None yet
Development

No branches or pull requests

8 participants