ibessonov commented on code in PR #1462:
URL: https://github.com/apache/ignite-3/pull/1462#discussion_r1054439523
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/storage/state/TxStateStorage.java:
##########
@@ -122,9 +125,55 @@ public interface TxStateStorage extends ManuallyCloseable {
void close();
/**
- * Removes all data from the storage.
+ * Closes and removes all data from the storage.
*
- * @throws StorageException In case when the operation has failed.
+ * @throws IgniteInternalException with {@link
Transactions#TX_STATE_STORAGE_ERR} error code in case when the operation has
failed.
*/
void destroy();
+
+ /**
+ * Prepares the transaction state storage for a full rebalance: clears the
storage, sets the {@link #lastAppliedIndex()} and
+ * {@link #lastAppliedTerm()} to {@link #FULL_REBALANCE_IN_PROGRESS}, and
closes all cursors.
+ *
+ * <p>After calling this method, only write methods will be available, and
read methods with {@link #lastApplied(long, long)} will
+ * throw {@link IgniteInternalException}.
Review Comment:
Can we have a more specific exception? Maybe a link to the corresponding
error code? Let's make our exceptions better.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/storage/state/TxStateStorage.java:
##########
@@ -122,9 +125,55 @@ public interface TxStateStorage extends ManuallyCloseable {
void close();
/**
- * Removes all data from the storage.
+ * Closes and removes all data from the storage.
*
- * @throws StorageException In case when the operation has failed.
+ * @throws IgniteInternalException with {@link
Transactions#TX_STATE_STORAGE_ERR} error code in case when the operation has
failed.
*/
void destroy();
+
+ /**
+ * Prepares the transaction state storage for a full rebalance: clears the
storage, sets the {@link #lastAppliedIndex()} and
+ * {@link #lastAppliedTerm()} to {@link #FULL_REBALANCE_IN_PROGRESS}, and
closes all cursors.
+ *
+ * <p>After calling this method, only write methods will be available, and
read methods with {@link #lastApplied(long, long)} will
+ * throw {@link IgniteInternalException}.
+ *
+ * <p>This method must be called before every full rebalance of
transaction state storage and ends with a call to one of the methods:
+ * <ul>
+ * <li>{@link #abortFullRebalance()} - in case of errors or
cancellation of a full rebalance;</li>
+ * <li>{@link #finishFullRebalance(long, long)} - in case of
successful completion of a full rebalance.</li>
+ * </ul>
+ *
+ * <p>If the {@link #lastAppliedIndex()} is {@link
#FULL_REBALANCE_IN_PROGRESS} after a node restart, then the storage needs to be
+ * cleared before it can be used.
Review Comment:
Ok, how do you clean it using this interface? Maybe we shouldn't mention it
like this, because cleaning happens automatically during the storage start,
isn't it?
##########
modules/transactions/src/testFixtures/java/org/apache/ignite/internal/tx/storage/state/AbstractTxStateStorageTest.java:
##########
@@ -257,53 +247,178 @@ public void
scanOnlySeesDataExistingAtTheMomentOfCreation() throws Exception {
List<UUID> txIdsReturnedByScan = cursor.stream()
.map(IgniteBiTuple::getKey)
.collect(toList());
+
assertThat(txIdsReturnedByScan, is(List.of(existingBeforeScan)));
}
-
- partitionStorage.close();
Review Comment:
This implies that partitions are closed by the table, right? That's cool
##########
modules/transactions/src/testFixtures/java/org/apache/ignite/internal/tx/storage/state/AbstractTxStateStorageTest.java:
##########
@@ -257,53 +247,178 @@ public void
scanOnlySeesDataExistingAtTheMomentOfCreation() throws Exception {
List<UUID> txIdsReturnedByScan = cursor.stream()
.map(IgniteBiTuple::getKey)
.collect(toList());
+
assertThat(txIdsReturnedByScan, is(List.of(existingBeforeScan)));
}
-
- partitionStorage.close();
}
@Test
void lastAppliedIndexGetterIsConsistentWithSetter() {
TxStateStorage partitionStorage =
tableStorage.getOrCreateTxStateStorage(0);
- try {
- partitionStorage.lastApplied(10, 2);
+ partitionStorage.lastApplied(10, 2);
- assertThat(partitionStorage.lastAppliedIndex(), is(10L));
- } finally {
- partitionStorage.close();
- }
+ assertThat(partitionStorage.lastAppliedIndex(), is(10L));
}
@Test
void lastAppliedTermGetterIsConsistentWithSetter() {
TxStateStorage partitionStorage =
tableStorage.getOrCreateTxStateStorage(0);
- try {
- partitionStorage.lastApplied(10, 2);
+ partitionStorage.lastApplied(10, 2);
- assertThat(partitionStorage.lastAppliedTerm(), is(2L));
- } finally {
- partitionStorage.close();
- }
+ assertThat(partitionStorage.lastAppliedTerm(), is(2L));
}
@Test
void compareAndSetMakesLastAppliedChangeVisible() {
TxStateStorage partitionStorage =
tableStorage.getOrCreateTxStateStorage(0);
- try {
- UUID txId = UUID.randomUUID();
- partitionStorage.compareAndSet(txId, null, randomTxMeta(1, txId),
10, 2);
+ UUID txId = UUID.randomUUID();
+ partitionStorage.compareAndSet(txId, null, randomTxMeta(1, txId), 10,
2);
+
+ assertThat(partitionStorage.lastAppliedIndex(), is(10L));
+ assertThat(partitionStorage.lastAppliedTerm(), is(2L));
+ }
+
+ @Test
+ public void testSuccessFullRebalance() throws Exception {
+ TxStateStorage storage = tableStorage.getOrCreateTxStateStorage(0);
+
+ // Nothing will happen because the full rebalance has not started.
+ storage.finishFullRebalance(100, 500).get(1, SECONDS);
Review Comment:
Are you sure that this one shouldn't throw an exception?
--
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]
