sk0x50 commented on code in PR #5276:
URL: https://github.com/apache/ignite-3/pull/5276#discussion_r1969814076
##########
modules/index/src/main/java/org/apache/ignite/internal/index/IndexBuildController.java:
##########
@@ -184,60 +224,116 @@ private CompletableFuture<?>
onIndexRemoved(RemoveIndexEventParameters parameter
private CompletableFuture<?>
onPrimaryReplicaElected(PrimaryReplicaEventParameters parameters) {
return inBusyLockAsync(busyLock, () -> {
- TablePartitionId primaryReplicaId = (TablePartitionId)
parameters.groupId();
-
if (isLocalNode(clusterService, parameters.leaseholderId())) {
- primaryReplicaIds.add(primaryReplicaId);
+ // This assert is added for testing purposes, at least for now.
+ assert (enabledColocation() && parameters.groupId() instanceof
ZonePartitionId)
+ || (!enabledColocation() && parameters.groupId()
instanceof TablePartitionId) :
+ "Primary replica ID must be of type ZonePartitionId if
colocation is enabled, "
+ + "otherwise it must be of type
TablePartitionId";
+
+ primaryReplicaIds.add(parameters.groupId());
// It is safe to get the latest version of the catalog because
the PRIMARY_REPLICA_ELECTED event is handled on the
// metastore thread.
Catalog catalog =
catalogService.catalog(catalogService.latestCatalogVersion());
- // TODO: IGNITE-22656 It is necessary not to generate an event
for a destroyed table by LWM
- if (catalog == null ||
catalog.table(primaryReplicaId.tableId()) == null) {
- return nullCompletedFuture();
- }
+ if (enabledColocation()) {
+ ZonePartitionId primaryReplicaId = (ZonePartitionId)
parameters.groupId();
+
+ CatalogZoneDescriptor zoneDescriptor =
catalog.zone(primaryReplicaId.zoneId());
+ // TODO: IGNITE-22656 It is necessary not to generate an
event for a destroyed zone by LWM
+ if (zoneDescriptor == null) {
+ return nullCompletedFuture();
+ }
- return getMvTableStorageFuture(parameters.causalityToken(),
primaryReplicaId)
- .thenCompose(mvTableStorage ->
awaitPrimaryReplica(primaryReplicaId, parameters.startTime())
- .thenAccept(replicaMeta ->
tryScheduleBuildIndexesForNewPrimaryReplica(
- catalog.version(),
- primaryReplicaId,
- mvTableStorage,
- replicaMeta
- ))
- );
+ var indexFutures = new ArrayList<CompletableFuture<?>>();
+ for (CatalogTableDescriptor tableDescriptor :
catalog.tables()) {
+ // 1. Perhaps, it makes sense to get primary replica
future first and then get table storage future,
+ // because, it will be the same for all tables in the
zone for the given partition.
+ // 2. Is it possible to filter out tables in efficient
way
+ // that do not have indices to avoid creating
unnecessary futures?
+ // It looks like
catalog.indexes(tableDescriptor.id()).isEmpty() is good enough.
+ // However, what about PK index?
+ CompletableFuture<?> future =
+
getMvTableStorageFuture(parameters.causalityToken(), tableDescriptor.id())
+ .thenCompose(mvTableStorage ->
awaitPrimaryReplica(primaryReplicaId, parameters.startTime())
+ .thenAccept(replicaMeta ->
tryScheduleBuildIndexesForNewPrimaryReplica(
+ catalog.version(),
+ tableDescriptor,
+ primaryReplicaId,
+ mvTableStorage,
+ replicaMeta)));
+
+ indexFutures.add(future);
+ }
+
+ return
allOf(indexFutures.toArray(CompletableFuture[]::new));
+ } else {
+ TablePartitionId primaryReplicaId = (TablePartitionId)
parameters.groupId();
+
+ // TODO: IGNITE-22656 It is necessary not to generate an
event for a destroyed table by LWM
+ CatalogTableDescriptor tableDescriptor =
catalog.table(primaryReplicaId.tableId());
+ if (tableDescriptor == null) {
+ return nullCompletedFuture();
+ }
+
+ return
getMvTableStorageFuture(parameters.causalityToken(), primaryReplicaId.tableId())
+ .thenCompose(mvTableStorage ->
awaitPrimaryReplica(primaryReplicaId, parameters.startTime())
+ .thenAccept(replicaMeta ->
tryScheduleBuildIndexesForNewPrimaryReplica(
+ catalog.version(),
+ tableDescriptor,
+ primaryReplicaId,
+ mvTableStorage,
+ replicaMeta
+ ))
+ );
+ }
} else {
- stopBuildingIndexesIfPrimaryExpired(primaryReplicaId);
+ stopBuildingIndexesIfPrimaryExpired(parameters.groupId());
return nullCompletedFuture();
}
});
}
private void tryScheduleBuildIndexesForNewPrimaryReplica(
+ // TODO It seems that we can pass the catalog itself instead of
its version.
int catalogVersion,
- TablePartitionId primaryReplicaId,
+ CatalogTableDescriptor tableDescriptor,
+ ReplicationGroupId primaryReplicaId,
MvTableStorage mvTableStorage,
ReplicaMeta replicaMeta
) {
inBusyLock(busyLock, () -> {
- if (isLeaseExpire(replicaMeta)) {
+ if (isLeaseExpired(replicaMeta)) {
stopBuildingIndexesIfPrimaryExpired(primaryReplicaId);
return;
}
Catalog catalog = catalogService.catalog(catalogVersion);
+ // This assert does not make sense, {@link CatalogService#catalog}
throws CatalogNotFoundException if the catalog is not found.
assert catalog != null : "Not found catalog for version " +
catalogVersion;
- for (CatalogIndexDescriptor indexDescriptor :
catalog.indexes(primaryReplicaId.tableId())) {
+ for (CatalogIndexDescriptor indexDescriptor :
catalog.indexes(tableDescriptor.id())) {
if (indexDescriptor.status() == BUILDING) {
- scheduleBuildIndex(primaryReplicaId, indexDescriptor,
mvTableStorage, enlistmentConsistencyToken(replicaMeta));
+ scheduleBuildIndex(
+ tableDescriptor.zoneId(),
+ tableDescriptor.id(),
+ enabledColocation()
+ ? ((ZonePartitionId)
primaryReplicaId).partitionId()
Review Comment:
Nice catch! Thank you!
--
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
