Skip to content

Commit c502479

Browse files
authored
Retry failed procedure rollback without losing state (#18822) (#18825)
* Retry failed procedure rollback without losing state * Fix missing TSStatusCode import in procedure manager test (cherry picked from commit 2751ee1)
1 parent bcb10ff commit c502479

6 files changed

Lines changed: 166 additions & 2 deletions

File tree

‎iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ProcedureManager.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1697,7 +1697,10 @@ private TSStatus waitingProcedureFinished(Procedure<?> procedure) {
16971697
&& System.currentTimeMillis() - startTimeForCurrentProcedure < PROCEDURE_WAIT_TIME_OUT) {
16981698
sleepWithoutInterrupt(PROCEDURE_WAIT_RETRY_TIMEOUT);
16991699
}
1700-
if (!procedure.isFinished()) {
1700+
// Conflict checks can fail before a procedure is assigned a persistent id. In that case the
1701+
// procedure remains at NO_PROC_ID but already carries the user-facing failure.
1702+
if (!procedure.isFinished()
1703+
&& !(procedure.getProcId() == Procedure.NO_PROC_ID && procedure.isFailed())) {
17011704
// The procedure is still executing
17021705
status =
17031706
RpcUtils.getStatus(TSStatusCode.INTERNAL_REQUEST_TIME_OUT, PROCEDURE_TIMEOUT_MESSAGE);

‎iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/Procedure.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,15 @@ protected boolean isYieldAfterExecution(Env env) {
265265
return false;
266266
}
267267

268+
/**
269+
* Delay in milliseconds before retrying a failed rollback. A negative value keeps the default
270+
* behavior of completing rollback after an exception. Procedures opting in must make rollback
271+
* idempotent and retain the state needed by the next attempt.
272+
*/
273+
protected long getRollbackRetryTimeout() {
274+
return -1;
275+
}
276+
268277
// -------------------------Internal methods - called by the procedureExecutor------------------
269278
final boolean tryAcquireExecution() {
270279
return executing.compareAndSet(false, true);
@@ -679,6 +688,11 @@ protected synchronized void setFailure(final ProcedureException exception) {
679688
*/
680689
protected synchronized boolean setTimeoutFailure(Env env) {
681690
if (state == ProcedureState.WAITING_TIMEOUT) {
691+
if (exception != null && getRollbackRetryTimeout() >= 0) {
692+
// Resume compensation without replacing the original execution failure.
693+
setState(ProcedureState.FAILED);
694+
return true;
695+
}
682696
long timeDiff = System.currentTimeMillis() - lastUpdate;
683697
setFailure(
684698
"ProcedureExecutor",

‎iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/ProcedureExecutor.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,15 +650,30 @@ private ProcedureLockState acquireLock(Procedure<Env> proc) {
650650
* @return procedure lock state
651651
*/
652652
private ProcedureLockState executeRollback(Procedure<Env> procedure) {
653+
boolean rollbackFailed = false;
653654
try {
654655
procedure.doRollback(this.environment);
655656
} catch (IOException e) {
657+
rollbackFailed = true;
656658
LOG.error("Roll back failed for {}", procedure, e);
657659
} catch (InterruptedException e) {
660+
rollbackFailed = true;
658661
LOG.warn("Interrupted exception occured for {}", procedure, e);
659662
} catch (Throwable t) {
663+
rollbackFailed = true;
660664
LOG.error("CODE-BUG: runtime exception for {}", procedure, t);
661665
}
666+
if (rollbackFailed && procedure.getRollbackRetryTimeout() >= 0) {
667+
procedure.setTimeout(procedure.getRollbackRetryTimeout());
668+
procedure.setState(ProcedureState.WAITING_TIMEOUT);
669+
try {
670+
store.update(procedure);
671+
} catch (Exception e) {
672+
LOG.warn(procedure.toString(), e);
673+
}
674+
timeoutExecutor.add(procedure);
675+
return ProcedureLockState.LOCK_EVENT_WAIT;
676+
}
662677
cleanupAfterRollback(procedure);
663678
return ProcedureLockState.LOCK_ACQUIRED;
664679
}

‎iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/procedure/impl/StateMachineProcedure.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,11 +234,15 @@ protected void rollback(final Env env)
234234
states.removeLast();
235235
}
236236

237+
boolean completed = false;
237238
try {
238239
updateTimestamp();
239240
rollbackState(env, getCurrentState());
241+
completed = true;
240242
} finally {
241-
states.removeLast();
243+
if ((completed || getRollbackRetryTimeout() < 0) && !states.isEmpty()) {
244+
states.removeLast();
245+
}
242246
updateTimestamp();
243247
}
244248
}

‎iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/manager/ProcedureManagerTest.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,18 @@
3030
import org.apache.iotdb.confignode.procedure.ProcedureExecutor;
3131
import org.apache.iotdb.confignode.procedure.env.ConfigNodeProcedureEnv;
3232
import org.apache.iotdb.confignode.procedure.env.RemoveDataNodeHandler;
33+
import org.apache.iotdb.confignode.procedure.exception.ProcedureException;
3334
import org.apache.iotdb.confignode.procedure.impl.node.RemoveDataNodesProcedure;
3435
import org.apache.iotdb.confignode.procedure.impl.region.RegionMigrateProcedure;
3536
import org.apache.iotdb.confignode.procedure.impl.region.RegionMigrationPlan;
37+
import org.apache.iotdb.rpc.TSStatusCode;
3638

3739
import org.junit.Assert;
3840
import org.junit.BeforeClass;
3941
import org.junit.Test;
4042

4143
import java.io.IOException;
44+
import java.lang.reflect.Method;
4245
import java.util.ArrayList;
4346
import java.util.Arrays;
4447
import java.util.HashMap;
@@ -49,6 +52,7 @@
4952
import java.util.concurrent.ConcurrentHashMap;
5053

5154
import static org.apache.iotdb.db.service.RegionMigrateService.isFailed;
55+
import static org.mockito.Mockito.mock;
5256
import static org.mockito.Mockito.spy;
5357
import static org.mockito.Mockito.when;
5458

@@ -141,6 +145,22 @@ public void testCheckRemoveDataNodeWithAnotherRemoveProcedure() {
141145
Assert.assertTrue(isFailed(status));
142146
}
143147

148+
@Test
149+
public void testPreSubmissionFailureWithNoProcedureIdIsReportedImmediately() throws Exception {
150+
final Procedure<ConfigNodeProcedureEnv> procedure = mock(Procedure.class);
151+
when(procedure.getProcId()).thenReturn(Procedure.NO_PROC_ID);
152+
when(procedure.isFinished()).thenReturn(false);
153+
when(procedure.isFailed()).thenReturn(true);
154+
when(procedure.getException()).thenReturn(new ProcedureException("conflict"));
155+
156+
final Method waitingProcedureFinished =
157+
ProcedureManager.class.getDeclaredMethod("waitingProcedureFinished", Procedure.class);
158+
waitingProcedureFinished.setAccessible(true);
159+
final TSStatus status =
160+
(TSStatus) waitingProcedureFinished.invoke(PROCEDURE_MANAGER, procedure);
161+
Assert.assertEquals(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode(), status.getCode());
162+
}
163+
144164
@Test
145165
public void testCheckRemoveDataNodeWithConflictRegionMigrateProcedure() {
146166
RegionMigrateProcedure regionMigrateProcedure =

‎iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/procedure/STMProcedureTest.java‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,25 @@
2121

2222
import org.apache.iotdb.confignode.procedure.entity.SimpleSTMProcedure;
2323
import org.apache.iotdb.confignode.procedure.env.TestProcEnv;
24+
import org.apache.iotdb.confignode.procedure.exception.ProcedureException;
2425
import org.apache.iotdb.confignode.procedure.impl.StateMachineProcedure;
2526
import org.apache.iotdb.confignode.procedure.state.ProcedureState;
2627
import org.apache.iotdb.confignode.procedure.util.ProcedureTestUtil;
2728

2829
import org.junit.Assert;
2930
import org.junit.Test;
3031

32+
import java.io.ByteArrayOutputStream;
33+
import java.io.DataOutputStream;
34+
import java.io.IOException;
3135
import java.lang.reflect.Field;
36+
import java.nio.ByteBuffer;
37+
import java.util.ArrayList;
38+
import java.util.Arrays;
39+
import java.util.List;
3240
import java.util.concurrent.ConcurrentLinkedDeque;
41+
import java.util.concurrent.CountDownLatch;
42+
import java.util.concurrent.TimeUnit;
3343
import java.util.concurrent.atomic.AtomicInteger;
3444

3545
public class STMProcedureTest extends TestProcedureBase {
@@ -60,6 +70,104 @@ public void testRolledBackProcedure() {
6070
Assert.assertEquals(1 + success - rolledback, acc.get());
6171
}
6272

73+
@Test
74+
public void testFailedBeforeExecutionCanRollbackWithoutState() throws Exception {
75+
final RetryingRollbackProcedure procedure = new RetryingRollbackProcedure();
76+
procedure.setFailure(procedure.originalFailure);
77+
procedure.doRollback(env);
78+
Assert.assertEquals(Arrays.asList(0), procedure.attemptedStates);
79+
Assert.assertSame(procedure.originalFailure, procedure.getException());
80+
}
81+
82+
@Test
83+
public void testFailedRollbackRetainsStateAfterSerialization() throws Exception {
84+
final RetryingRollbackProcedure procedure = new RetryingRollbackProcedure();
85+
procedure.setState(ProcedureState.RUNNABLE);
86+
procedure.doExecute(env);
87+
procedure.doExecute(env);
88+
try {
89+
procedure.doRollback(env);
90+
Assert.fail("Compensation should fail while the Region is unavailable");
91+
} catch (IOException expected) {
92+
Assert.assertEquals(Arrays.asList(1), procedure.attemptedStates);
93+
}
94+
procedure.setTimeout(1000);
95+
procedure.setState(ProcedureState.WAITING_TIMEOUT);
96+
final ByteArrayOutputStream bytes = new ByteArrayOutputStream();
97+
procedure.serialize(new DataOutputStream(bytes));
98+
final RetryingRollbackProcedure restored = new RetryingRollbackProcedure();
99+
restored.deserialize(ByteBuffer.wrap(bytes.toByteArray()));
100+
Assert.assertTrue(restored.isFailed());
101+
Assert.assertEquals(
102+
procedure.originalFailure.getMessage(), restored.getException().getMessage());
103+
restored.attemptedStates.addAll(Arrays.asList(1, 1));
104+
restored.doRollback(env);
105+
restored.doRollback(env);
106+
Assert.assertEquals(Arrays.asList(1, 1, 1, 0), restored.attemptedStates);
107+
}
108+
109+
@Test
110+
public void testFailedRollbackRetriesSameStateWithoutOverwritingFailure() throws Exception {
111+
final RetryingRollbackProcedure procedure = new RetryingRollbackProcedure();
112+
final long procId = procExecutor.submitProcedure(procedure);
113+
Assert.assertTrue(procedure.firstFailure.await(5, TimeUnit.SECONDS));
114+
Assert.assertTrue(procedure.completed.await(5, TimeUnit.SECONDS));
115+
ProcedureTestUtil.waitForProcedure(procExecutor, procId);
116+
Assert.assertTrue(procedure.isFinished());
117+
Assert.assertEquals(Arrays.asList(1, 1, 1, 0), procedure.attemptedStates);
118+
Assert.assertSame(procedure.originalFailure, procedure.getException());
119+
}
120+
121+
private static class RetryingRollbackProcedure
122+
extends StateMachineProcedure<TestProcEnv, Integer> {
123+
private final CountDownLatch firstFailure = new CountDownLatch(1);
124+
private final CountDownLatch completed = new CountDownLatch(1);
125+
private final List<Integer> attemptedStates = new ArrayList<>();
126+
private final ProcedureException originalFailure = new ProcedureException("Execution failed");
127+
128+
@Override
129+
protected Flow executeFromState(TestProcEnv env, Integer state) {
130+
if (state == 0) {
131+
setNextState(1);
132+
return Flow.HAS_MORE_STATE;
133+
}
134+
setFailure(originalFailure);
135+
return Flow.NO_MORE_STATE;
136+
}
137+
138+
@Override
139+
protected void rollbackState(TestProcEnv env, Integer state) throws IOException {
140+
attemptedStates.add(state);
141+
if (state == 1 && attemptedStates.size() < 3) {
142+
firstFailure.countDown();
143+
throw new IOException("Region temporarily unavailable");
144+
}
145+
if (state == 0) {
146+
completed.countDown();
147+
}
148+
}
149+
150+
@Override
151+
protected long getRollbackRetryTimeout() {
152+
return 50;
153+
}
154+
155+
@Override
156+
protected Integer getState(int stateId) {
157+
return stateId;
158+
}
159+
160+
@Override
161+
protected int getStateId(Integer state) {
162+
return state;
163+
}
164+
165+
@Override
166+
protected Integer getInitialState() {
167+
return 0;
168+
}
169+
}
170+
63171
@Test
64172
public void testEofStateReexecutionDoesNotCallExecuteFromState() throws Exception {
65173
EofReexecutionProcedure procedure = new EofReexecutionProcedure();

0 commit comments

Comments
 (0)