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

feat(csharp/src/Drivers/Apache): add connect and query timeout options #2312

Conversation

birschick-bq
Copy link
Contributor

@birschick-bq birschick-bq commented Nov 8, 2024

Adds options for command and query timeout

Property Description Default
adbc.spark.connect_timeout_ms Sets the timeout (in milliseconds) to open a new session. Values can be 0 (infinite) or greater than zero. 30000
adbc.apache.statement.query_timeout_s Sets the maximum time (in seconds) for a query to complete. Values can be 0 (infinite) or greater than zero. 60

@github-actions github-actions bot added this to the ADBC Libraries 16 milestone Nov 11, 2024
@birschick-bq birschick-bq marked this pull request as draft November 15, 2024 16:37
Copy link
Contributor Author

@birschick-bq birschick-bq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. One small nit (that I should have done already).

csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs Outdated Show resolved Hide resolved
csharp/src/Drivers/Apache/Spark/SparkStatement.cs Outdated Show resolved Hide resolved
@birschick-bq birschick-bq changed the title feat(csharp/src/Drivers/Apache): add connect and query timeout options - unimplemented feat(csharp/src/Drivers/Apache): add connect and query timeout options Nov 20, 2024
@birschick-bq birschick-bq marked this pull request as ready for review November 21, 2024 00:31
@birschick-bq birschick-bq marked this pull request as draft November 25, 2024 20:03
@birschick-bq birschick-bq marked this pull request as draft November 26, 2024 18:43
@birschick-bq
Copy link
Contributor Author

Converted to draft because I'm trying to resolve failing tests

@davidhcoe
Copy link
Contributor

Adds options for command and query timeout

Property Description Default
adbc.spark.connect_timeout_ms Sets the timeout (in milliseconds) to open a new session. Values can be -1 (infinite) or greater than zero. 30000
adbc.statement.query_timeout_s Sets the maximum time (in seconds) for a query to complete. Values can be -1 (infinite) or greater than zero. 60
TODO: Dynamically update HTTP request timeout when statement option is updated dynamically.

These have changed now.

@birschick-bq
Copy link
Contributor Author

@CurtHagenlocher - A lot of churn on this PR - but I'd say it is ready for review, if you have the time. Thanks.

@birschick-bq birschick-bq marked this pull request as ready for review November 27, 2024 18:15
@birschick-bq
Copy link
Contributor Author

HI @CurtHagenlocher - let me know if you have some time to review this PR. Thanks, again.

Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement! I've left a bunch of feedback.

@@ -114,10 +115,31 @@ public override CommandType CommandType
}
}

/// <summary>
/// Gets or setts the name of the command timeout property for the underlying ADBC driver.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: sets

That said, I feel like this is a dubious design choice. If there were a standard for command timeout properties in ADBC then we could hook into that from the wrapper. But if the consumer needs to have custom code anyway in order to set the command timeout, then I think we're better off just exposing the ability to set statement options through DbCommand -- it's equally usable for this case, and more generally useful for other unanticipated cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that a particular driver may define this in a unit other than seconds given the lack of a standard.

As a suggestion, if the property name were on the ADO.NET connection string, it could be used much more transparently because the consumer will already have to have a generic mechanism for setting connection string properties. With this API, the consumer has to know explicitly that they're using a wrapped ADBC driver in order to be able to use the CommandTimeout API.

csharp/src/Drivers/Apache/ApacheParameters.cs Outdated Show resolved Hide resolved
csharp/src/Drivers/Apache/ApacheUtility.cs Outdated Show resolved Hide resolved
csharp/src/Drivers/Apache/ApacheUtility.cs Outdated Show resolved Hide resolved
csharp/src/Drivers/Apache/Hive2/HiveServer2Connection.cs Outdated Show resolved Hide resolved
csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs Outdated Show resolved Hide resolved
csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs Outdated Show resolved Hide resolved
csharp/test/Drivers/Apache/Spark/ClientTests.cs Outdated Show resolved Hide resolved
@birschick-bq birschick-bq marked this pull request as draft December 3, 2024 18:15
@birschick-bq birschick-bq marked this pull request as ready for review December 3, 2024 19:57
@CurtHagenlocher
Copy link
Contributor

Note: because we can't pass the same cancellation token to ExecuteQueryAsync and the ReadNextBatchAsync, the runtime may exceed the query timeout

Can linked cancellation tokens be used for this?

@@ -114,10 +116,32 @@ public override CommandType CommandType
}
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra blank line

Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good overall. I do think the API for making the CommandTimeout work should be a little more consumer-friendly by allowing it to be set on the connection string. And for consistency, we should probably support DbConnection.ConnectionTimeout in a similar way. But these could be done in a followup. Remove that extra blank line and let me know if you want to check this in without those changes.

Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@CurtHagenlocher CurtHagenlocher merged commit 845f9c2 into apache:main Dec 4, 2024
6 checks passed
CurtHagenlocher pushed a commit that referenced this pull request Dec 6, 2024
…ng (#2352)

Follow up to #2312 to add
parsing of the timeout values and other custom properties like
StructBehavior and DecimalBehavior from the connection string.

---------

Co-authored-by: David Coe <[email protected]>
Co-authored-by: Bruce Irschick <[email protected]>
@birschick-bq birschick-bq deleted the dev/birschick-bq/connect-query-timeout-stub branch December 10, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants