Copilot commented on code in PR #7989:
URL: https://github.com/apache/ignite-3/pull/7989#discussion_r3116586155
##########
modules/platforms/dotnet/Apache.Ignite.Tests/PartitionAwarenessTests.cs:
##########
@@ -416,6 +422,54 @@ await AssertOpOnNode(
expectedNode);
}
+ [Test]
+ [TestCaseSource(nameof(PartitionNodeCases))]
+ public async Task TestExecutePartitionedRoutesRequestToPrimaryNode(int
partitionId, int node)
+ {
+ using var client = await GetClient();
+ var expectedNode = node == 1 ? _server1 : _server2;
+
+ var jobTarget = JobTarget.Partition(FakeServer.ExistingTableName, new
Internal.Table.HashPartition(partitionId));
+ var jobDescriptor = new JobDescriptor<object?, object?>("job");
+ var jobExecution = await client.Compute.SubmitAsync(jobTarget,
jobDescriptor, null);
+
Review Comment:
`jobExecution` is assigned but never used. If this is meant as a warm-up,
consider just awaiting the call without storing the result (or use the
execution to fetch the result/status) to avoid leaving an extra job execution
unobserved in the test.
```suggestion
// Warm up.
await client.Compute.SubmitAsync(jobTarget, jobDescriptor, null);
```
##########
modules/platforms/dotnet/Apache.Ignite/Compute/JobTarget.cs:
##########
@@ -122,6 +122,29 @@ public static IJobTarget<TKey> Colocated<TKey>(string
tableName, TKey key, IMapp
where TKey : notnull =>
Colocated(QualifiedName.Parse(tableName), key, mapper);
+ /// <summary>
+ /// Creates a job target for a specific partition.
+ /// </summary>
+ /// <param name="tableName">Table name.</param>
+ /// <param name="partition">The Partition.</param>
+ /// <returns>Partition job target.</returns>
+ public static IJobTarget<IPartition> Partition(QualifiedName tableName,
IPartition partition)
+ {
+ IgniteArgumentCheck.NotNull(partition);
+
+ return new PartitionTarget(tableName, partition);
+ }
Review Comment:
`JobTarget.Partition` currently requires an `IPartition` instance, but the
only `IPartition` implementation in the client appears to be
`Internal.Table.HashPartition` (internal), so public API consumers can't create
a target from a known partition id. Consider adding a public overload like
`Partition(QualifiedName tableName, long partitionId)` (or `int`) that creates
the internal partition wrapper, or otherwise providing a public way to
construct an `IPartition` from an id. This also aligns with the PR description
which mentions a `(QualifiedName, int partitionId)` overload.
##########
modules/platforms/dotnet/Apache.Ignite.Tests/Compute/ComputeTests.cs:
##########
@@ -140,6 +140,33 @@ public async Task TestBroadcastAllNodes()
Assert.AreEqual(nodes[3].Name + "123", await
res4.GetResultAsync());
}
+ [Test]
+ public async Task TestBroadcastTable()
+ {
+ var table = await Client.Tables.GetTableAsync(TableName);
+ var partitions = await
table!.PartitionDistribution.GetPartitionsAsync();
+ var partitionIds = partitions.Select(p => p.Id).Order();
+
+ IBroadcastExecution<long> broadcastExecution = await
Client.Compute.SubmitBroadcastAsync(
+ BroadcastJobTarget.Table(TableName),
+ GetPartitionJob,
+ null);
+
+ long[] taskResults = await Task
+ .WhenAll(broadcastExecution.JobExecutions.Select(x =>
x.GetResultAsync()));
+
+ Assert.AreEqual(partitionIds, taskResults.Order());
+ }
+
+ [Test]
+ public void TestBroadcastTableOverloads()
+ {
+ var target1 = BroadcastJobTarget.Table(TableName);
+ var target2 =
BroadcastJobTarget.Table(QualifiedName.Parse(TableName));
+ Assert.AreEqual(target1.Data.ObjectName, target2.Data.ObjectName);
+ Assert.AreEqual(target1.Data.ObjectName, target2.Data.ObjectName);
Review Comment:
There is a duplicated assertion here (`ObjectName` compared twice), which
likely reduces test coverage of the overload behavior. Replace the second
assertion with a different check (e.g., compare `SchemaName`, `CanonicalName`,
or assert the whole `QualifiedName` is equal).
```suggestion
Assert.AreEqual(target1.Data.SchemaName,
target2.Data.SchemaName);
```
##########
modules/platforms/dotnet/Apache.Ignite.Tests/FakeServer.cs:
##########
@@ -772,14 +774,20 @@ private void GetSchemas(MsgPackReader reader, Socket
handler, long requestId)
Send(handler, requestId, arrayBufferWriter);
}
- private PooledArrayBuffer ComputeExecute(MsgPackReader reader, bool
colocated = false)
+ private PooledArrayBuffer ComputeExecute(MsgPackReader reader,
ClientOp optCode)
{
Review Comment:
Minor naming/typo: the parameter is called `optCode` but represents the
operation code (`opCode`). Renaming improves readability and avoids confusion
with "options".
--
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]
