Copilot commented on code in PR #5753:
URL: https://github.com/apache/ignite-3/pull/5753#discussion_r2073782322
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/executor/platform/dotnet/DotNetComputeExecutor.java:
##########
@@ -110,7 +114,22 @@ private CompletableFuture<ComputeJobDataHolder>
executeJobAsync(
long jobId = jobIdGen.incrementAndGet();
return getPlatformComputeConnectionWithRetryAsync()
- .thenCompose(conn -> conn.executeJobAsync(jobId,
deploymentUnitPaths, jobClassName, input));
+ .thenCompose(conn -> conn.executeJobAsync(jobId,
deploymentUnitPaths, jobClassName, input))
Review Comment:
[nitpick] Inside the exception handler block, consider logging the original
exception details (or stack trace) before wrapping it to aid in debugging
unforeseen errors.
##########
modules/platforms/dotnet/Apache.Ignite.Tests/Compute/PlatformComputeTests.cs:
##########
@@ -32,6 +34,11 @@ namespace Apache.Ignite.Tests.Compute;
/// </summary>
public class PlatformComputeTests : IgniteTestsBase
{
Review Comment:
[nitpick] Consider adding a clarifying comment for JobRunnerJob to explain
its purpose and the composition of its identifier, ensuring its intent is clear
to future maintainers.
```suggestion
{
/// <summary>
/// Descriptor for the JobRunner job, which is used to execute compute
jobs on the platform.
/// The identifier is composed of the platform test node runner prefix
and the job name ("$JobRunnerJob").
/// </summary>
```
##########
modules/platforms/dotnet/Apache.Ignite.Tests/ClientSocketTests.cs:
##########
@@ -80,12 +80,15 @@ public async Task TestDisposedSocketThrowsExceptionOnSend()
[Test]
public void TestConnectWithoutServerThrowsException()
{
- Assert.CatchAsync(async () => await
ClientSocket.ConnectAsync(GetEndPoint(569), new(), Listener));
+ Assert.CatchAsync(async () => await
ClientSocket.ConnectAsync(GetEndPoint(569), GetConfigInternal(), Listener));
}
private static SocketEndpoint GetEndPoint(int? serverPort = null) =>
new(new(IPAddress.Loopback, serverPort ?? ServerPort),
string.Empty, string.Empty);
Review Comment:
[nitpick] The GetConfigInternal method passes a null IgniteApiAccessor via
Task.FromResult((IgniteApiAccessor)null!), which may be intentional for
testing, but consider adding a comment to clarify this usage.
```suggestion
// Returns a configuration with a null IgniteApiAccessor for testing
purposes.
// This simulates a scenario where the accessor is not required or
unavailable.
```
--
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]
