rpuch commented on code in PR #7644:
URL: https://github.com/apache/ignite-3/pull/7644#discussion_r2847450045
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildTask.java:
##########
@@ -276,13 +291,17 @@ private CompletableFuture<Void> handleNextBatch(@Nullable
RowId highestRowId) {
if (!(cause instanceof ReplicationTimeoutException
|| cause instanceof GroupOverloadedException)) {
return
CompletableFuture.<Void>failedFuture(cause);
}
- } else if (indexStorage.getNextRowIdToBuild() == null)
{
- // Index has been built.
- LOG.info("Index build completed [{}]",
createCommonIndexInfo());
+ } else {
Review Comment:
Would it make sense to keep `else if` structure here for consistency? As
current `else` is actually an if/else itself
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItBuildIndexTest.java:
##########
@@ -116,6 +119,46 @@ void testBuildIndexOnStableTopology(int replicas) throws
Exception {
.check();
}
+ @Test
+ void testRaftCommandsDuplicationOnPrimaryNotCollocatedWithLeader() throws
Exception {
+ int replicas = 2;
+ int partitions = 1;
Review Comment:
Could you please explain the numbers? Like 'we have 3 nodes, so one node
will be primary, one will be Raft leader, and one will not host the partition'?
Or somehow else?
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItBuildIndexTest.java:
##########
@@ -116,6 +119,46 @@ void testBuildIndexOnStableTopology(int replicas) throws
Exception {
.check();
}
+ @Test
+ void testRaftCommandsDuplicationOnPrimaryNotCollocatedWithLeader() throws
Exception {
+ int replicas = 2;
+ int partitions = 1;
+
+ LogInspector logInspector = new LogInspector(
+ BuildIndexCommandHandler.class.getName(),
+ evt ->
evt.getMessage().getFormattedMessage().contains("Duplicated building the index
command received")
+ );
+
+ logInspector.start();
+ try {
+ createAndPopulateTable(replicas, partitions);
+
+ ZonePartitionId tableGroupId = replicationGroupId(TABLE_NAME, 0);
+
+ IgniteImpl currentPrimary = primaryReplica(tableGroupId);
+
+ changeLeader(tableGroupId, currentPrimary);
Review Comment:
It seems important that the leader is chosen to be different from the
primary. Please encode this either in the method name, or add a comment
##########
modules/index/src/integrationTest/java/org/apache/ignite/internal/index/ItBuildIndexTest.java:
##########
@@ -116,6 +119,46 @@ void testBuildIndexOnStableTopology(int replicas) throws
Exception {
.check();
}
+ @Test
+ void testRaftCommandsDuplicationOnPrimaryNotCollocatedWithLeader() throws
Exception {
Review Comment:
```suggestion
void testRaftCommandsDuplicationOnPrimaryNotColocatedWithLeader() throws
Exception {
```
While working on colocation feature, we decided to spell it with just 1 'l'
--
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]
