You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/03/20 11:32:48 UTC
Re: [PR] IGNITE-22990 Treat schema change in which index appears and starts build as incompatible [ignite-3]
Copilot commented on code in PR #7815:
URL: https://github.com/apache/ignite-3/pull/7815#discussion_r2965273410
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/schema/CatalogValidationSchemasSource.java:
##########
@@ -155,56 +166,48 @@ private Stream<SimpleEntry<Catalog, CatalogTableDescriptor>> tableVersionsBetwee
.takeWhile(Objects::nonNull);
}
- private static FullTableSchema fullSchemaFromCatalog(Catalog catalog, CatalogTableDescriptor tableDescriptor) {
+ private FullTableSchema fullSchemaFromCatalog(Catalog catalog, CatalogTableDescriptor tableDescriptor) {
assert tableDescriptor != null;
+ Int2IntMap indexesJustStartedBeingBuilt = indexesJustStartedBeingBuilt(tableDescriptor, catalog);
+
return new FullTableSchema(
catalog.version(),
tableDescriptor.latestSchemaVersion(),
tableDescriptor.id(),
tableDescriptor.name(),
- tableDescriptor.columns()
+ tableDescriptor.columns(),
+ indexesJustStartedBeingBuilt
);
}
- private static class CatalogVersionsSpan {
- private final int tableId;
- private final int fromCatalogVersion;
- private final int toCatalogVersion;
-
- private CatalogVersionsSpan(int tableId, int fromCatalogVersion, int toCatalogVersion) {
- this.tableId = tableId;
- this.fromCatalogVersion = fromCatalogVersion;
- this.toCatalogVersion = toCatalogVersion;
- }
+ private Int2IntMap indexesJustStartedBeingBuilt(CatalogTableDescriptor tableDescriptor, Catalog catalog) {
+ Int2IntMap indexesJustStartedBeingBuilt = new Int2IntOpenHashMap();
- @Override
- public boolean equals(Object o) {
- if (this == o) {
- return true;
- }
- if (o == null || getClass() != o.getClass()) {
- return false;
- }
+ for (CatalogIndexDescriptor index : catalog.indexes(tableDescriptor.id())) {
+ if (index.status() == CatalogIndexStatus.BUILDING) {
Review Comment:
`indexesJustStartedBeingBuilt` always allocates an `Int2IntOpenHashMap` even when there are no BUILDING indexes. Since this method is called for every catalog version processed, consider allocating lazily (start with `Int2IntMaps.EMPTY_MAP` / `null` and create the map only on the first match) to reduce allocations and GC pressure.
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/schema/CatalogValidationSchemasSource.java:
##########
@@ -155,56 +166,48 @@ private Stream<SimpleEntry<Catalog, CatalogTableDescriptor>> tableVersionsBetwee
.takeWhile(Objects::nonNull);
}
- private static FullTableSchema fullSchemaFromCatalog(Catalog catalog, CatalogTableDescriptor tableDescriptor) {
+ private FullTableSchema fullSchemaFromCatalog(Catalog catalog, CatalogTableDescriptor tableDescriptor) {
assert tableDescriptor != null;
+ Int2IntMap indexesJustStartedBeingBuilt = indexesJustStartedBeingBuilt(tableDescriptor, catalog);
+
return new FullTableSchema(
catalog.version(),
tableDescriptor.latestSchemaVersion(),
tableDescriptor.id(),
tableDescriptor.name(),
- tableDescriptor.columns()
+ tableDescriptor.columns(),
+ indexesJustStartedBeingBuilt
);
}
- private static class CatalogVersionsSpan {
- private final int tableId;
- private final int fromCatalogVersion;
- private final int toCatalogVersion;
-
- private CatalogVersionsSpan(int tableId, int fromCatalogVersion, int toCatalogVersion) {
- this.tableId = tableId;
- this.fromCatalogVersion = fromCatalogVersion;
- this.toCatalogVersion = toCatalogVersion;
- }
+ private Int2IntMap indexesJustStartedBeingBuilt(CatalogTableDescriptor tableDescriptor, Catalog catalog) {
+ Int2IntMap indexesJustStartedBeingBuilt = new Int2IntOpenHashMap();
- @Override
- public boolean equals(Object o) {
- if (this == o) {
- return true;
- }
- if (o == null || getClass() != o.getClass()) {
- return false;
- }
+ for (CatalogIndexDescriptor index : catalog.indexes(tableDescriptor.id())) {
+ if (index.status() == CatalogIndexStatus.BUILDING) {
+ IndexMeta indexMeta = indexMetasAccess.indexMeta(index.id());
+ if (indexMeta == null) {
+ throw new IllegalStateException(
+ "Index " + index.id() + " is not in the index metastore even though it's being built, according to the catalog"
+ );
+ }
- CatalogVersionsSpan that = (CatalogVersionsSpan) o;
+ MetaIndexStatusChange changeToBuilding = indexMeta.statusChange(MetaIndexStatus.BUILDING);
+ if (changeToBuilding == null) {
+ throw new IllegalStateException(
+ "Index " + index.id() + " is being built, but its status was never changed to BUILDING"
+ );
+ }
- if (tableId != that.tableId) {
- return false;
- }
- if (fromCatalogVersion != that.fromCatalogVersion) {
- return false;
+ if (catalog.version() == changeToBuilding.catalogVersion()) {
+ // The index has just become being built.
+ indexesJustStartedBeingBuilt.put(index.id(), indexMeta.statusChange(MetaIndexStatus.REGISTERED).catalogVersion());
+ }
Review Comment:
`IndexMeta.statusChange(...)` never returns null (it throws `IllegalArgumentException` when absent), so the `changeToBuilding == null` branch is dead code and the thrown exception will currently bypass this more descriptive error message. Consider using `statusChangeNullable` (and handling null explicitly) or catching `IllegalArgumentException` to rethrow with context; same applies to `statusChange(REGISTERED)` which can also throw and currently isn't guarded.
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/schemacompat/SchemaCompatibilityValidator.java:
##########
@@ -138,14 +141,26 @@ private CompatValidationResult validateForwardSchemaCompatibility(
assert !tableSchemas.isEmpty() : "No table schemas for table " + tableId + " between " + beginTimestamp + " and " + commitTimestamp;
+ int initialSchemaCatalogVersion = tableSchemas.get(0).catalogVersion();
+ int commitSchemaCatalogVersion = tableSchemas.get(tableSchemas.size() - 1).catalogVersion();
+
+ return forwardCompatValidationResultCache.computeIfAbsent(
+ new CatalogVersionsSpan(tableId, initialSchemaCatalogVersion, commitSchemaCatalogVersion),
+ key -> validateForwardSchemaCompatibility(initialSchemaCatalogVersion, tableSchemas)
+ );
Review Comment:
`forwardCompatValidationResultCache` is now keyed by `(tableId, fromCatalogVersion, toCatalogVersion)`, which can create many more cache entries than the previous per-adjacent-diff cache (potentially O(n^2) spans per table as transactions commit across varying ranges). If this cache is expected to be long-lived (see the existing TODO about compaction), consider returning to per-step caching for context-free validators and applying the context-dependent check (`IndexStartedBuildingValidator`) without caching (or use a bounded/evicting cache) to avoid unbounded growth.
```suggestion
return validateForwardSchemaCompatibility(initialSchemaCatalogVersion, tableSchemas);
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org