abonander commented on code in PR #3905:
URL: https://github.com/apache/arrow-adbc/pull/3905#discussion_r2848950227
##########
rust/core/src/sync.rs:
##########
@@ -44,6 +44,24 @@ pub trait Optionable {
fn get_option_double(&self, key: Self::Option) -> Result<f64>;
}
+/// A handle to cancel an in-progress operation on a connection.
+///
+/// This is a separated handle because otherwise it would be impossible to
+/// call a `cancel` method on a connection or statement itself.
+pub trait CancelHandle: Send {
Review Comment:
https://github.com/apache/arrow-adbc/blob/e8642f824393a99d4a2ac3cde89f15b10e7f3101/c/include/arrow-adbc/adbc.h#L2133-L2134
"thread-safe" implies that this handle should be `Sync` as well since there
isn't really a distinction between "can be moved between threads" and "can be
called concurrently from multiple threads" in C.
##########
rust/core/src/sync.rs:
##########
@@ -44,6 +44,24 @@ pub trait Optionable {
fn get_option_double(&self, key: Self::Option) -> Result<f64>;
}
+/// A handle to cancel an in-progress operation on a connection.
+///
+/// This is a separated handle because otherwise it would be impossible to
+/// call a `cancel` method on a connection or statement itself.
+pub trait CancelHandle: Send {
+ /// Cancel the in-progress operation on a connection.
+ fn try_cancel(&self) -> Result<()>;
+}
+
+/// A cancellation handle that does nothing (because cancellation is
unsupported).
+pub struct NoOpCancellationHandle;
+
+impl CancelHandle for NoOpCancellationHandle {
+ fn try_cancel(&self) -> Result<()> {
+ Ok(())
Review Comment:
Should the default be to return `ADBC_STATUS_UNKNOWN` to indicate that the
operation cannot be cancelled per
https://github.com/apache/arrow-adbc/blob/e8642f824393a99d4a2ac3cde89f15b10e7f3101/c/include/arrow-adbc/adbc.h#L2143
##########
rust/core/src/sync.rs:
##########
@@ -44,6 +44,24 @@ pub trait Optionable {
fn get_option_double(&self, key: Self::Option) -> Result<f64>;
}
+/// A handle to cancel an in-progress operation on a connection.
+///
+/// This is a separated handle because otherwise it would be impossible to
+/// call a `cancel` method on a connection or statement itself.
+pub trait CancelHandle: Send {
+ /// Cancel the in-progress operation on a connection.
+ fn try_cancel(&self) -> Result<()>;
Review Comment:
Are the `try_` prefix and the default to `NoOpCancellationHandle` meant to
imply that this is on a best-effort basis only? Can that be more clearly
documented?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
