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

Add SQL command timeout option to database loader. #4494

Open
wants to merge 4 commits into
base: master
from

Conversation

@ashbhandare
Copy link
Contributor

ashbhandare commented Nov 21, 2019

Fixes #4484

This change adds a way to specify the SQL command timeout https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlcommand.commandtimeout?view=netframework-4.8
while creating the DatabaseSource.

@ashbhandare ashbhandare requested a review from dotnet/mlnet-core as a code owner Nov 21, 2019
@@ -14,7 +14,8 @@ public sealed class DatabaseSource
/// <param name="providerFactory">The factory used to create the <see cref="DbConnection"/>..</param>
/// <param name="connectionString">The string used to open the connection.</param>
/// <param name="commandText">The text command to run against the data source.</param>
public DatabaseSource(DbProviderFactory providerFactory, string connectionString, string commandText)
/// <param name="commandTimeout">The time in seconds to wait for the command to execute. Default is 30 sec.</param>
public DatabaseSource(DbProviderFactory providerFactory, string connectionString, string commandText, int commandTimeout = 30)

This comment has been minimized.

Copy link
@eerhardt

eerhardt Nov 21, 2019

Member

This is a breaking change. You can't add a parameter (even an optional parameter) to an already shipped method.

You will need to create a new overload to add this new property. #Resolved

This comment has been minimized.

Copy link
@ashbhandare

ashbhandare Nov 22, 2019

Author Contributor

changing


In reply to: 349252694 [](ancestors = 349252694)

@@ -79,6 +79,63 @@ public void IrisLightGbm()
}).PredictedLabel);
}

[LightGBMFact]
public void IrisLightGbmConnectionTimeout()

This comment has been minimized.

Copy link
@eerhardt

eerhardt Nov 21, 2019

Member

How is this testing the timeout? #Resolved

This comment has been minimized.

Copy link
@ashbhandare

ashbhandare Nov 22, 2019

Author Contributor

This just tests that a command timeout be specified, what would you recommend that the test look like to confirm that it is being exercised?


In reply to: 349252966 [](ancestors = 349252966)

This comment has been minimized.

Copy link
@eerhardt

eerhardt Nov 22, 2019

Member

I'm not exactly sure. But my initial thinking is doing something that would make the command timeout, and ensuring the correct behavior occurs. #Resolved

This comment has been minimized.

Copy link
@ashbhandare

ashbhandare Nov 22, 2019

Author Contributor

added test where SQL query has delay added, and command timeout is exercised.


In reply to: 349721564 [](ancestors = 349721564)

This comment has been minimized.

Copy link
@veikkoeeva

veikkoeeva Nov 28, 2019

Contributor

Should you also test for user initiated cancellation? I wonder if this way of testing introduces test failures on the infra or problems with other databases than SQL Server?

As for an example, here is one another way: https://github.com/dotnet/orleans/blob/585680f3c06cb82f22e07b65330b769d333a8aae/test/Extensions/TesterAdoNet/StorageTests/RelationalStoreTests.cs#L179. (For context, this is actually testing timeouts while streaming data. This piece isn't the best possible otherwise, but should give perhaps food for thoughts in the future.)

This comment has been minimized.

Copy link
@ashbhandare

ashbhandare Dec 2, 2019

Author Contributor

Should you also test for user initiated cancellation? I wonder if this way of testing introduces test failures on the infra or problems with other databases than SQL Server?

As for an example, here is one another way: https://github.com/dotnet/orleans/blob/585680f3c06cb82f22e07b65330b769d333a8aae/test/Extensions/TesterAdoNet/StorageTests/RelationalStoreTests.cs#L179. (For context, this is actually testing timeouts while streaming data. This piece isn't the best possible otherwise, but should give perhaps food for thoughts in the future.)

Could you elaborate on what 'user-initiated cancellation' would mean in this context?

@eerhardt eerhardt requested a review from tannergooding Nov 21, 2019
@@ -14,7 +14,8 @@ public sealed class DatabaseSource
/// <param name="providerFactory">The factory used to create the <see cref="DbConnection"/>..</param>
/// <param name="connectionString">The string used to open the connection.</param>
/// <param name="commandText">The text command to run against the data source.</param>
public DatabaseSource(DbProviderFactory providerFactory, string connectionString, string commandText)
/// <param name="commandTimeout">The time in seconds to wait for the command to execute. Default is 30 sec.</param>
public DatabaseSource(DbProviderFactory providerFactory, string connectionString, string commandText, int commandTimeout = 30)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Nov 21, 2019

Member

It's likely worth making it explicitly clear that this is in seconds. #Resolved

This comment has been minimized.

Copy link
@ashbhandare

ashbhandare Nov 22, 2019

Author Contributor

The XML description of the parameter does state that the time is in seconds already


In reply to: 349254631 [](ancestors = 349254631)

This comment has been minimized.

Copy link
@veikkoeeva

veikkoeeva Nov 28, 2019

Contributor

If I may, as a bit of an outsider, for a reason or another it's not always easy to see XML comments. So being explicit like commandTimeoutInSeconds = 30 is easier to catch. I believe it's sort of a pattern used quite commonly too. (It's a pedantic nit, but maybe also using the whole word 'seconds' or the SI unit?, https://physics.nist.gov/cuu/Units/checklist.html) #Resolved

This comment has been minimized.

Copy link
@ashbhandare

ashbhandare Dec 2, 2019

Author Contributor

Wouldn't matching the name of the actual property that is being set be more consistent. See https://docs.microsoft.com/en-us/dotnet/api/system.data.sqlclient.sqlcommand.commandtimeout?view=netcore-3.0.


In reply to: 351692404 [](ancestors = 351692404)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Dec 2, 2019

Member

The ML API here is not, strictly speaking, tied to SQL. It is meant to be usable with any database provider. It is more "tied" to DbCommand.CommandTimeout: https://docs.microsoft.com/en-us/dotnet/api/system.data.common.dbcommand.commandtimeout?view=netframework-4.8#System_Data_Common_DbCommand_CommandTimeout

Adding InSeconds just helps clarify the unit of measurement that the timeout is (which can be particularly helpful as .NET APIs frequently deal in milliseconds instead). #Resolved

This comment has been minimized.

Copy link
@ashbhandare

ashbhandare Dec 2, 2019

Author Contributor

changing.


In reply to: 352740161 [](ancestors = 352740161)

ashbhandare added 3 commits Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.