rpuch commented on code in PR #5568:
URL: https://github.com/apache/ignite-3/pull/5568#discussion_r2028572841
##########
modules/partition-replicator/src/integrationTest/java/org/apache/ignite/internal/partition/replicator/ItReplicaLifecycleTest.java:
##########
@@ -669,6 +671,28 @@ public void testReplicaSafeTimeSyncRequest() throws
Exception {
checkSafeTimeWasAdjustedForZoneGroup(node1, zoneId, partId);
}
+ @Test
+ public void testListenersAreRemovedOnTableDrop() throws Exception {
Review Comment:
```suggestion
public void testListenersAreRemovedOnTableDestruction() throws Exception
{
```
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/raft/ZonePartitionRaftListener.java:
##########
@@ -297,7 +297,18 @@ private CommandResult processTableAwareCommand(
long commandTerm,
@Nullable HybridTimestamp safeTimestamp
) {
- return tableProcessors.get(tableId).processCommand(command,
commandIndex, commandTerm, safeTimestamp);
+ RaftTableProcessor tableProcessor = tableProcessors.get(tableId);
+
+ if (tableProcessor == null) {
Review Comment:
Same concern as with replica requests
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/TableManagerTest.java:
##########
@@ -448,9 +446,9 @@ public void testDropTable() throws Exception {
assertNull(tableManager.table(DYNAMIC_TABLE_FOR_DROP_NAME));
assertEquals(0, tableManager.tables().size());
- verify(mvTableStorage, atMost(0)).destroy();
- verify(txStateStorage, atMost(0)).destroy();
- verify(replicaMgr, atMost(0)).stopReplica(any());
+ verify(mvTableStorage, never()).destroy();
+ verify(txStateStorage, never()).destroy();
+ verify(replicaMgr, never()).stopReplica(any());
Review Comment:
On line 456, `txStateStorage.destroy()` should not be called, but it is,
according to the verification
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java:
##########
@@ -239,10 +239,24 @@ private CompletableFuture<ReplicaResult>
processTableAwareRequest(
ReplicaPrimacy replicaPrimacy,
UUID senderId
) {
- int tableId = ((TableAware) request).tableId();
-
return
tableAwareReplicaRequestPreProcessor.preProcessTableAwareRequest(request,
replicaPrimacy, senderId)
- .thenCompose(ignored -> replicas.get(tableId).process(request,
replicaPrimacy, senderId));
+ .thenCompose(ignored -> {
+ int tableId = ((TableAware) request).tableId();
+
+ ReplicaTableProcessor replicaProcessor =
replicaProcessors.get(tableId);
+
+ if (replicaProcessor == null) {
+ // Most of the times this condition should be false.
This logging message is added in case a request got stuck
+ // somewhere while being replicated and arrived on
this node after the target table had been removed. In this case
+ // we ignore the command, which should be safe to do,
because the underlying storage was destroyed anyway.
+ LOG.warn("Replica processor for table ID {} not found.
Command will be ignored: {}", tableId,
Review Comment:
Currently, it seems that there is a bug (maybe this one
https://issues.apache.org/jira/browse/IGNITE-24991) because of which a request
arrives not AFTER removal, but BEFORE addition. We still need this to trigger
an NPE. If we apply your change, we'll stop seeing these NPEs. I'm not sure
this is what we should do
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
