sashapolo commented on code in PR #7815:
URL: https://github.com/apache/ignite-3/pull/7815#discussion_r2974957262
##########
modules/api/src/main/java/org/apache/ignite/lang/ErrorGroups.java:
##########
@@ -454,7 +454,12 @@ public static class Transactions {
/** Operation failed because the transaction is already finished. */
public static final int TX_ALREADY_FINISHED_ERR =
TX_ERR_GROUP.registerErrorCode((short) 13);
- /** Failure due to a stale operation of a completed transaction is
detected. */
+ /**
+ * Failure due to a stale operation of a completed transaction is
detected.
+ *
+ * @deprecated This error is no longer used.
+ */
+ @Deprecated
Review Comment:
Why can't we remove it right away?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/schema/TableDefinitionDiff.java:
##########
@@ -38,6 +42,8 @@ public class TableDefinitionDiff {
private final List<CatalogTableColumnDescriptor> removedColumns;
private final List<ColumnDefinitionDiff> changedColumns;
+ private final Int2IntMap indexesJustStartedBeingBuilt;
Review Comment:
Please add a comment about what this map contains, the name is not
descriptive enough
##########
modules/partition-replicator/src/test/java/org/apache/ignite/internal/partition/replicator/schemacompat/SchemaCompatibilityValidatorTest.java:
##########
@@ -378,6 +381,47 @@ private void
assertChangeIsNotBackwardCompatible(SchemaChangeSource changeSource
assertThat(result.toSchemaVersion(), is(2));
}
+ @Test
+ void doesNotCacheStaleResultForDifferentInitialCatalogVersions() {
Review Comment:
Can you please add a clarification javadoc? While the test is well
documented, I struggle to understand the test name and why this particular
scenario is imporant
##########
modules/schema-sync/src/integrationTest/java/org/apache/ignite/internal/schemasync/ItBlockedSchemaSyncAndRaftCommandExecutionTest.java:
##########
@@ -85,8 +89,11 @@ void operationBlockedOnSchemaSyncDoesNotPreventNodeStop()
throws Exception {
assertTimeoutPreemptively(Duration.ofSeconds(10), () ->
cluster.stopNode(0));
- //noinspection ThrowableNotThrown
- assertWillThrowCausedBy(inhibitorAndFuture.future,
NodeStoppingException.class);
+ Exception ex = assertWillThrow(inhibitorAndFuture.future,
Exception.class);
Review Comment:
Why did you change this code?
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/schemacompat/SchemaCompatibilityValidator.java:
##########
@@ -482,4 +509,24 @@ public ValidationResult compatible(ColumnDefinitionDiff
diff) {
: new ValidationResult(ValidatorVerdict.INCOMPATIBLE,
"Column type changed incompatibly");
}
}
+
+ private static class IndexStartedBuildingValidator implements
ForwardCompatibilityValidator {
+ @Override
+ public ValidationResult compatible(TableDefinitionDiff diff,
ValidationContext context) {
+ Int2IntMap indexesJustStartedBeingBuilt =
diff.indexesJustStartedBeingBuilt();
+ if (indexesJustStartedBeingBuilt.isEmpty()) {
+ return ValidationResult.DONT_CARE;
+ }
+
+ for (int indexId : indexesJustStartedBeingBuilt.keySet()) {
+ int registeredCatalogVersion =
indexesJustStartedBeingBuilt.get(indexId);
+
+ if (context.initialSchemaCatalogVersion <
registeredCatalogVersion) {
+ return new ValidationResult(ValidatorVerdict.INCOMPATIBLE,
"Index was both created and started being built");
Review Comment:
> Index was both created and started being built
What does it mean? Why is that an error? The message is quite confusing
--
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]
