Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch closed pull request #5611: IGNITE-24981 Partition storage is missing on index build with colocation URL: https://github.com/apache/ignite-3/pull/5611 -- 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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch commented on PR #5611: URL: https://github.com/apache/ignite-3/pull/5611#issuecomment-2801935798 Superceded by #5612 -- 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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch merged PR #5612: URL: https://github.com/apache/ignite-3/pull/5612 -- 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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
sashapolo commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2042100387
##
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##
@@ -535,30 +534,36 @@ private CompletableFuture
createZoneReplicationNodes(
int catalogVersion,
int partitionCount
) {
-return inBusyLockAsync(busyLock, () -> {
-assert assignments != null : IgniteStringFormatter.format("Zone
has empty assignments [id={}].", zoneId);
-
-var partitionsStartFutures = new
CompletableFuture[assignments.size()];
-
-for (int partId = 0; partId < assignments.size(); partId++) {
-Assignments zoneAssignment = assignments.get(partId);
-
-Assignment localMemberAssignment =
localMemberAssignment(zoneAssignment);
-
-var zonePartitionId = new ZonePartitionId(zoneId, partId);
-
-partitionsStartFutures[partId] =
createZonePartitionReplicationNode(
-zonePartitionId,
-localMemberAssignment,
-zoneAssignment,
-causalityToken,
-partitionCount,
-isVolatileZoneForCatalogVersion(zoneId, catalogVersion)
-);
-}
+// 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:
I guess it would make more sense to move this assertion out of the closures.
--
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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch commented on code in PR #5612: URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039718924 ## 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: If we do it, then pending assignments change events handling (inside the same zone) will be sequential. Imagine that partitions 1, 2, 3 of the same zone are now added to pending assignments of node A. If we just take write lock for any of them, others will need to wait for it. But current approach allows to keep this parallelism -- 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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch commented on code in PR #5612: URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2040002744 ## 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: Also, when a new zone is created, its partitions would be started sequentially if we took a write lock for each of them -- 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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
sashapolo commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039780918
##
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##
@@ -3069,13 +3068,18 @@ public void
setStreamerReceiverRunner(StreamerReceiverRunner runner) {
}
public Set zoneTables(int zoneId) {
-return tablesPerZone.computeIfAbsent(zoneId, id -> new HashSet<>());
+return Set.copyOf(zoneTablesRawSet(zoneId));
Review Comment:
If we are making defensive copies now, wouldn't it make sense to simply
store a COW set instead of a concurrent one?
--
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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039791573
##
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##
@@ -3069,13 +3068,18 @@ public void
setStreamerReceiverRunner(StreamerReceiverRunner runner) {
}
public Set zoneTables(int zoneId) {
-return tablesPerZone.computeIfAbsent(zoneId, id -> new HashSet<>());
+return Set.copyOf(zoneTablesRawSet(zoneId));
Review Comment:
We only make defensive copies for external users (that is, for
DisasterRecoveryManager), these calls are extremely rare. For table/partition
operations, mutable set seems to be a cheaper approach
--
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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039764925
##
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##
@@ -3069,13 +3068,15 @@ public void
setStreamerReceiverRunner(StreamerReceiverRunner runner) {
}
public Set 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:
No, we don't rely on set 'sharedness'. I agree, `getOrDefault()` is better,
I changed the code.
I also made another change: public `zoneTables()` now makes a defensive copy.
--
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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039758323
##
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##
@@ -535,8 +534,11 @@ private CompletableFuture 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:
Tried to fix it
--
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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
sashapolo commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039742971
##
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##
@@ -3069,13 +3068,15 @@ public void
setStreamerReceiverRunner(StreamerReceiverRunner runner) {
}
public Set 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:
> 10 calls with getOrDefault() will produce 10 instances of an empty set
Well, I expected `getOrDefault` to return `Set.of()` as default, so there
will be only a single instance. It's strange that we currently populate the map
during a `get` operation. Or do we somewhere rely on the fact that this is a
shared set and some data will appear in it?
--
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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
rpuch commented on code in PR #5612:
URL: https://github.com/apache/ignite-3/pull/5612#discussion_r2039727217
##
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##
@@ -3069,13 +3068,15 @@ public void
setStreamerReceiverRunner(StreamerReceiverRunner runner) {
}
public Set 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:
If the zone has no tables, 10 calls with `getOrDefault()` will produce 10
instances of an empty set, while for `computeIfAbsent()` it will be just 1
instance. On the other hand, if we have a million empty zones, `getOrAbsent()`
will hog less memory.
Both cases seem to be almost identical in real life. Does `getOrDefault()`
have any significant advantages?
--
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]
Re: [PR] IGNITE-24981 Partition storage is missing on index build with colocation [ignite-3]
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 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 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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
