You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2023/05/16 10:09:57 UTC
[GitHub] [ignite-3] rpuch opened a new pull request, #2075: IGNITE-19454 Validate backwards schema compatibility
rpuch opened a new pull request, #2075:
URL: https://github.com/apache/ignite-3/pull/2075
https://issues.apache.org/jira/browse/IGNITE-19454
--
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
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #2075: IGNITE-19454 Validate backwards schema compatibility
Posted by "SammyVimes (via GitHub)" <gi...@apache.org>.
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
[GitHub] [ignite-3] rpuch commented on a diff in pull request #2075: IGNITE-19454 Validate backwards schema compatibility
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #2075:
URL: https://github.com/apache/ignite-3/pull/2075#discussion_r1197671401
##########
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:
I rephrased the javadoc, please take a look, is it better now?
--
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
[GitHub] [ignite-3] rpuch commented on a diff in pull request #2075: IGNITE-19454 Validate backwards schema compatibility
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #2075:
URL: https://github.com/apache/ignite-3/pull/2075#discussion_r1197530767
##########
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:
We can identify the version that corresponds to the end of the interval by either a ts or the version ID explicitly. In the case of the backwards validation, we already know the ID, and it seems safer to use the real version ID instead of the tuple's commitTs (as the real schema ID might be different from the one that corresponds to the commitTs).
Between means that we first identify version IDs corresponding to the start and the end of the interval, then we return the list of all versions between them (including), or an empty list if start ID turns out to be greater than end ID.
--
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
[GitHub] [ignite-3] SammyVimes commented on a diff in pull request #2075: IGNITE-19454 Validate backwards schema compatibility
Posted by "SammyVimes (via GitHub)" <gi...@apache.org>.
SammyVimes commented on code in PR #2075:
URL: https://github.com/apache/ignite-3/pull/2075#discussion_r1197772803
##########
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:
Hm, ok
--
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
[GitHub] [ignite-3] SammyVimes merged pull request #2075: IGNITE-19454 Validate backwards schema compatibility
Posted by "SammyVimes (via GitHub)" <gi...@apache.org>.
SammyVimes merged PR #2075:
URL: https://github.com/apache/ignite-3/pull/2075
--
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