feat(csharp/src/Drivers/BigQuery): implement AdbcStatement.Cancel on BigQuery #3422
Conversation
|
@davidhcoe - Please review when you have a chance. |
|
Checking to see if the semantics should be that once a statement is cancelled - it stays cancelled? Or should the Cancel only apply when an execution is active? Additionally. The docs for Cancel says any streams should also be cancelled. Should I try to implement that behavior, as well? |
While this may not all be spelled out explicitly, the intent for a statement is clearly that a single statement should only be used to execute one query at a time and that a single statement may be used to execute multiple different queries consecutively. From this perspective, cancellation should only affect the currently-executing query. Cancelling the current query should not prevent the statement from being used to execute a new query.
Ideally, yes. |
Okay. That is much more clear. |
| private readonly CancellationRegistry cancellationRegistry; | ||
| private readonly CancellationTokenSource cancellationTokenSource; | ||
|
|
||
| private bool _disposedValue; |
There was a problem hiding this comment.
By usage, this code base has over twice as many _disposeds as _disposedValues (though we could normalize that in a separate PR).
There was a problem hiding this comment.
(In the same vein, this class has two fields that don't start with an underscore and one which does. And here too is another inconsistency across the code base :(.)
There was a problem hiding this comment.
Sorry. It is autogenerated code that I should change. I'll create a PR to normalize to _disposed from _disposedValue.
| Job = job; | ||
| } | ||
|
|
||
| public BigQueryJob? Job { get; set; } |
There was a problem hiding this comment.
Should the setter throw an exception if the existing value is non-null?
There was a problem hiding this comment.
Actually, the ability to swap out a Job mid-context is by design. It allows multiple calls/steps to share the same context.
| } | ||
| } | ||
|
|
||
| public static bool ContainsException<T>(Exception exception, out T? containedException) where T : Exception |
There was a problem hiding this comment.
FWIW, this could share an implementation with the other overload via e.g.
public static bool ContainsException<T>(Exception exception, out T? containedException) where T: Exception
{
if (ContainsException(exception, typeof(T), out Exception? found))
{
containedException = (T)found;
return true;
}
containedException = default;
return false;
}
3 participants
Adds implementation of
AdbcStatement.Cancelto the BigQuery driver.CancellationRegistrytracksCancellationContextCancellationContext.JobCancellationContextis registered, it will attempt to cancel the job on the server.