sashapolo commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039629404
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -598,7 +601,8 @@ private CompletableFuture<?>
createZonePartitionReplicationNode(
Assignments stableAssignments,
long revision,
int partitionCount,
- boolean isVolatileZone
+ boolean isVolatileZone,
+ boolean holdingZoneWriteLock
Review Comment:
Why don't we always call this method under lock instead?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -3069,13 +3068,15 @@ public void
setStreamerReceiverRunner(StreamerReceiverRunner runner) {
}
public Set<TableImpl> zoneTables(int zoneId) {
- return tablesPerZone.computeIfAbsent(zoneId, id -> new HashSet<>());
+ // Using a concurrent set as a value as it can be read (and iterated
over) without any synchronization by external callers.
+ return tablesPerZone.computeIfAbsent(zoneId, id ->
ConcurrentHashMap.newKeySet());
Review Comment:
Shouldn't it be `getOrDefault` instead?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -535,8 +534,11 @@ private CompletableFuture<Void> createZoneReplicationNodes(
int catalogVersion,
int partitionCount
) {
- return inBusyLockAsync(busyLock, () -> {
- assert assignments != null : IgniteStringFormatter.format("Zone
has empty assignments [id={}].", zoneId);
+ // Zone nodes might be created in the same catalog version as a table
in this zone, so there could be a race
+ // between storage creation due to replica start and storage creation
due to table addition. To eliminate the race,
+ // we acquire a write lock on the zone (table addition acquires a read
lock).
+ return inBusyLockAsync(busyLock, () ->
executeUnderZoneWriteLock(zoneId, () -> inBusyLockAsync(busyLock, () -> {
+ assert assignments != null : IgniteStringFormatter.format("Zone has
empty assignments [id={}].", zoneId);
Review Comment:
Something is not right with this formatting
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -535,8 +534,11 @@ private CompletableFuture<Void> createZoneReplicationNodes(
int catalogVersion,
int partitionCount
) {
- return inBusyLockAsync(busyLock, () -> {
- assert assignments != null : IgniteStringFormatter.format("Zone
has empty assignments [id={}].", zoneId);
+ // Zone nodes might be created in the same catalog version as a table
in this zone, so there could be a race
+ // between storage creation due to replica start and storage creation
due to table addition. To eliminate the race,
+ // we acquire a write lock on the zone (table addition acquires a read
lock).
+ return inBusyLockAsync(busyLock, () ->
executeUnderZoneWriteLock(zoneId, () -> inBusyLockAsync(busyLock, () -> {
+ assert assignments != null : IgniteStringFormatter.format("Zone has
empty assignments [id={}].", zoneId);
Review Comment:
```
inBusyLockAsync(busyLock, () -> executeUnderZoneWriteLock(zoneId, () ->
inBusyLockAsync(busyLock,
```
This looks so awkward...
--
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
