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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
