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