rpuch commented on code in PR #7500:
URL: https://github.com/apache/ignite-3/pull/7500#discussion_r2811035610
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/PartitionCommandListenerTest.java:
##########
@@ -392,18 +417,82 @@ void
updatesLastAppliedForCommandsUpdatingLastAppliedIndex(WriteCommand command)
verify(mvPartitionStorage).lastApplied(3, 2);
}
+ @ParameterizedTest
+ @MethodSource("fullUpdateCommands")
+ void
returnsFailedValidationResultIfSchemaChangeValidationOn1PcCommitFails(WriteCommand
command) {
+ when(validationSchemasSource.tableSchemaVersionsBetween(anyInt(),
any(HybridTimestamp.class), any(HybridTimestamp.class)))
+ .thenReturn(incompatibleSchemaChange());
+
+ CommandResult commandResult = commandListener.processCommand(command,
3, 2, hybridClock.now());
+
+ assertThat(commandResult.wasApplied(), is(true));
+ UpdateCommandResult updateCommandResult = (UpdateCommandResult)
commandResult.result();
+ assertThat(updateCommandResult, is(notNullValue()));
+ assertThat(updateCommandResult,
is(instanceOf(UpdateCommandResult.class)));
+ CompatibilityValidationResult compatibilityValidationResult =
updateCommandResult.compatibilityValidationResult();
+ assertThat(compatibilityValidationResult, is(notNullValue()));
+ assertThat(compatibilityValidationResult.isSuccessful(), is(false));
+ }
+
+ @ParameterizedTest
+ @MethodSource("fullUpdateCommands")
+ void
doesNotUpdateStorageIfSchemaChangeValidationOn1PcCommitFails(WriteCommand
command) {
+ when(validationSchemasSource.tableSchemaVersionsBetween(anyInt(),
any(HybridTimestamp.class), any(HybridTimestamp.class)))
+ .thenReturn(incompatibleSchemaChange());
+
+ commandListener.processCommand(command, 3, 2, hybridClock.now());
+
+ verify(mvPartitionStorage, never()).addWriteCommitted(any(), any(),
any());
+ }
+
+ @ParameterizedTest
+ @MethodSource("fullUpdateCommands")
+ void
updatesLastAppliedIndexIfSchemaChangeValidationOn1PcCommitFails(WriteCommand
command) {
+ when(validationSchemasSource.tableSchemaVersionsBetween(anyInt(),
any(HybridTimestamp.class), any(HybridTimestamp.class)))
+ .thenReturn(incompatibleSchemaChange());
+
+ commandListener.processCommand(command, 3, 2, hybridClock.now());
+
+ verify(mvPartitionStorage).lastApplied(3, 2);
+ }
+
+ private static Stream<Arguments> fullUpdateCommands() {
+ return Stream.of(updateCommand(true), updateAllCommand(true))
+ .map(Arguments::of);
+ }
+
+ private static List<FullTableSchema> incompatibleSchemaChange() {
+ CatalogTableColumnDescriptor column1 = new
CatalogTableColumnDescriptor("column1", ColumnType.INT32, false, 0, 0, 0, null);
+ CatalogTableColumnDescriptor column2 = new
CatalogTableColumnDescriptor("column2", ColumnType.INT32, false, 0, 0, 0, null);
+
+ // Dropping a column is a forward-incompatible schema change.
Review Comment:
If a schema change is forward-compatible (like adding a nullable/defaulted
column), a transaction started before the change CAN be committed after the
change. In such a case, the binary tuple will be formatted in the old format
(with old schema), but commitTs will point to the new one.
But for forward-incompatible changes, we cannot commit a transaction that
started on the old schema, but attempts to be committed on the new one. The
whole issue is about preventing such illegal commits (which can currently
happen for 1PC transactions, even though they should be forbidden).
--
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]
