Copilot commented on code in PR #7860:
URL: https://github.com/apache/ignite-3/pull/7860#discussion_r2981968304
##########
modules/platforms/dotnet/Apache.Ignite/Internal/Proto/ClientOpExtensions.cs:
##########
@@ -82,5 +82,16 @@ internal static class ClientOpExtensions
ClientOp.None or _ => throw new
ArgumentOutOfRangeException(nameof(op), op, message: null)
};
}
+
+ /// <summary>
+ /// Returns true for operations that can be cancelled via <see
cref="ClientOp.OperationCancel"/>.
+ /// </summary>
+ /// <param name="op">Op.</param>
Review Comment:
The XML doc for the new method uses a non-descriptive param description
("Op.") while nearby methods use more specific wording (e.g., "Operation
code."). Consider updating this to a clearer description to keep docs
consistent and useful.
```suggestion
/// <param name="op">Operation code.</param>
```
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientSocket.cs:
##########
@@ -739,8 +739,8 @@ private async Task<PooledBuffer> DoOutInOpAsyncInternal(
}
}
- private CancellationTokenRegistration RegisterCancellation(long
requestId, CancellationToken cancellationToken) =>
- cancellationToken == CancellationToken.None
+ private CancellationTokenRegistration RegisterCancellation(long
requestId, ClientOp op, CancellationToken cancellationToken) =>
+ cancellationToken == CancellationToken.None || !op.IsCancellable()
? default
: cancellationToken.Register(() => _ =
CancelRequestAsync(requestId));
Review Comment:
New behavior: cancellation callbacks are now registered only for ops
supported by OPERATION_CANCEL. There are existing tests that inspect
CancellationTokenSource registrations (e.g., SqlTests/TestUtils), but nothing
appears to cover the negative case (non-cancellable op should not register a
callback / should not attempt OPERATION_CANCEL). Adding a focused test would
help prevent regressions when new ClientOp values are added or server-side
cancelability changes.
##########
modules/platforms/dotnet/Apache.Ignite/Internal/ClientSocket.cs:
##########
@@ -739,8 +739,8 @@ private async Task<PooledBuffer> DoOutInOpAsyncInternal(
}
}
- private CancellationTokenRegistration RegisterCancellation(long
requestId, CancellationToken cancellationToken) =>
- cancellationToken == CancellationToken.None
+ private CancellationTokenRegistration RegisterCancellation(long
requestId, ClientOp op, CancellationToken cancellationToken) =>
+ cancellationToken == CancellationToken.None || !op.IsCancellable()
? default
: cancellationToken.Register(() => _ =
CancelRequestAsync(requestId));
Review Comment:
This change stops registering a cancellation callback for non-cancellable
ops, so the comment inside CancelRequestAsync about "Some operations can't be
canceled, so the server will ignore the cancellation request" is now misleading
(CancelRequestAsync should only be invoked for ops that support
OPERATION_CANCEL). Please update/clarify that comment to match the new behavior.
--
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]
