You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "SammyVimes (via GitHub)" <gi...@apache.org> on 2023/05/18 07:50:08 UTC
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #2075: IGNITE-19454 Validate backwards schema compatibility
SammyVimes commented on code in PR #2075:
URL: https://github.com/apache/ignite-3/pull/2075#discussion_r1196297245
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/CompatValidationResult.java:
##########
@@ -22,10 +22,10 @@
import org.jetbrains.annotations.Nullable;
/**
- * Result of forward schema compatibility validation.
+ * Result of a schema compatibility validation.
*/
-public class ForwardValidationResult {
- private static final ForwardValidationResult SUCCESS = new ForwardValidationResult(true, null, -1, -1);
+public class CompatValidationResult {
Review Comment:
Btw, should it throw on successful? Assertion should be enough
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/SchemaCompatValidator.java:
##########
@@ -98,20 +98,66 @@ private ForwardValidationResult validateSchemaCompatibility(
assert !tableSchemas.isEmpty();
for (int i = 0; i < tableSchemas.size() - 1; i++) {
- FullTableSchema from = tableSchemas.get(i);
- FullTableSchema to = tableSchemas.get(i + 1);
- if (!isCompatible(from, to)) {
- return ForwardValidationResult.failure(tableId, from.schemaVersion(), to.schemaVersion());
+ FullTableSchema oldSchema = tableSchemas.get(i);
+ FullTableSchema newSchema = tableSchemas.get(i + 1);
+ if (!isForwardCompatible(oldSchema, newSchema)) {
+ return CompatValidationResult.failure(tableId, oldSchema.schemaVersion(), newSchema.schemaVersion());
}
}
- return ForwardValidationResult.success();
+ return CompatValidationResult.success();
}
- private boolean isCompatible(FullTableSchema prevSchema, FullTableSchema nextSchema) {
+ private boolean isForwardCompatible(FullTableSchema prevSchema, FullTableSchema nextSchema) {
TableDefinitionDiff diff = nextSchema.diffFrom(prevSchema);
- // TODO: IGNITE-19229 - more sophisticated logic
+ // TODO: IGNITE-19229 - more sophisticated logic.
return diff.isEmpty();
}
+
+ /**
+ * Performs backward compatibility validation. That is, for a tuple that was just read in a transaction,
+ * checks that, if the tuple was written with a schema version later than the initial schema version
+ * of the transaction, the initial schema version is backward compatible with the tuple schema version.
Review Comment:
```suggestion
/**
* Performs backward compatibility validation for a tuple that was just read in a transaction.
* This method checks if the tuple was written using a schema version later than the initial schema version
* of the transaction, and verifies that the initial schema version is backward compatible with the tuple's schema version.
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/SchemaCompatValidator.java:
##########
@@ -98,20 +98,66 @@ private ForwardValidationResult validateSchemaCompatibility(
assert !tableSchemas.isEmpty();
for (int i = 0; i < tableSchemas.size() - 1; i++) {
- FullTableSchema from = tableSchemas.get(i);
- FullTableSchema to = tableSchemas.get(i + 1);
- if (!isCompatible(from, to)) {
- return ForwardValidationResult.failure(tableId, from.schemaVersion(), to.schemaVersion());
+ FullTableSchema oldSchema = tableSchemas.get(i);
+ FullTableSchema newSchema = tableSchemas.get(i + 1);
+ if (!isForwardCompatible(oldSchema, newSchema)) {
+ return CompatValidationResult.failure(tableId, oldSchema.schemaVersion(), newSchema.schemaVersion());
}
}
- return ForwardValidationResult.success();
+ return CompatValidationResult.success();
}
- private boolean isCompatible(FullTableSchema prevSchema, FullTableSchema nextSchema) {
+ private boolean isForwardCompatible(FullTableSchema prevSchema, FullTableSchema nextSchema) {
TableDefinitionDiff diff = nextSchema.diffFrom(prevSchema);
- // TODO: IGNITE-19229 - more sophisticated logic
+ // TODO: IGNITE-19229 - more sophisticated logic.
return diff.isEmpty();
}
+
+ /**
+ * Performs backward compatibility validation. That is, for a tuple that was just read in a transaction,
+ * checks that, if the tuple was written with a schema version later than the initial schema version
+ * of the transaction, the initial schema version is backward compatible with the tuple schema version.
+ *
+ * @param tupleSchemaVersion Schema version ID of the tuple.
+ * @param tableId ID of the table to which the tuple belongs.
+ * @param txId ID of the transaction that gets validated.
+ * @return Future of validation result.
+ */
+ CompletableFuture<CompatValidationResult> validateBackwards(int tupleSchemaVersion, UUID tableId, UUID txId) {
+ HybridTimestamp beginTimestamp = TransactionIds.beginTimestamp(txId);
+
+ return schemas.waitForSchemasAvailability(beginTimestamp)
+ .thenCompose(ignored -> schemas.waitForSchemaAvailability(tableId, tupleSchemaVersion))
+ .thenApply(ignored -> validateBackwardSchemaCompatibility(tupleSchemaVersion, tableId, beginTimestamp));
+ }
+
+ private CompatValidationResult validateBackwardSchemaCompatibility(
+ int tupleSchemaVersion,
+ UUID tableId,
+ HybridTimestamp beginTimestamp
+ ) {
+ List<FullTableSchema> tableSchemas = schemas.tableSchemaVersionsBetween(tableId, beginTimestamp, tupleSchemaVersion);
Review Comment:
Wait, we use both timestamp and version? What does 'between' mean then?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/replicator/SchemaCompatValidator.java:
##########
@@ -49,9 +49,9 @@ class SchemaCompatValidator {
* @param txId ID of the transaction that gets validated.
* @param enlistedGroupIds IDs of the partitions that are enlisted with the transaction.
* @param commitTimestamp Commit timestamp (or {@code null} if it's an abort).
- * @return Future completed with validation result.
+ * @return Future of validation result.
*/
- CompletableFuture<ForwardValidationResult> validateForwards(
+ CompletableFuture<CompatValidationResult> validateForwards(
Review Comment:
Should it be "validateForward" instead of validateForward**s**?
--
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