smengcl commented on code in PR #11190:
URL: https://github.com/apache/ozone/pull/11190#discussion_r4069229782
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -796,4 +833,53 @@ public void awaitDoubleBufferFlush() throws
InterruptedException {
public OzoneManagerDoubleBuffer getOzoneManagerDoubleBuffer() {
return ozoneManagerDoubleBuffer;
}
+
+ private void pauseApplyTransaction() {
+ synchronized (applyTransactionMonitor) {
+ if (applyTransactionPaused) {
+ return;
+ }
+ applyTransactionPaused = true;
+ boolean interrupted = false;
+ while (inFlightApplyTransactions > 0) {
+ try {
+ applyTransactionMonitor.wait();
+ } catch (InterruptedException ex) {
+ LOG.warn("Interrupted while waiting for in-flight apply transactions
to complete.");
+ interrupted = true;
+ }
+ }
+ if (interrupted) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ }
+
+ private void resumeApplyTransaction() {
+ synchronized (applyTransactionMonitor) {
+ if (!applyTransactionPaused) {
+ return;
+ }
+ applyTransactionPaused = false;
+ applyTransactionMonitor.notifyAll();
+ }
+ }
+
+ private void enterApplyTransaction() throws InterruptedException {
+ synchronized (applyTransactionMonitor) {
+ while (applyTransactionPaused) {
+ applyTransactionMonitor.wait();
Review Comment:
[P1] This can block the thread responsible for unpausing.
Ratis 3.2.1's post-install path calls `pause()` followed by
`reloadStateMachine()`. If `StateMachineUpdater` has already selected a
committed entry and enters `applyTransaction()` after pause, it blocks here.
That same updater must return to its loop and call `reinitialize()` →
`unpause()`, so neither side can make progress.
Please keep the updater able to reach reload and add coverage for this
interleaving. The current test calls `unpause()` from another thread, which
misses the production dependency.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -548,6 +573,13 @@ public synchronized void pause() {
getLifeCycle().transition(LifeCycle.State.PAUSED);
}
+ pauseApplyTransaction();
Review Comment:
[P1] Waiting for applies here can deadlock with an in-flight prepare request.
`pause()` is synchronized, so it retains the state-machine monitor while
waiting for `inFlightApplyTransactions` to reach zero. An in-flight
`OMPrepareRequest` calls `takeSnapshotAndPurgeLogs()` → `takeSnapshot()` →
synchronized `takeSnapshotImpl()`, which needs that same monitor. Pause waits
for prepare to finish, while prepare waits for pause to release the monitor.
Waiting on `applyTransactionMonitor` releases only that monitor, not `this`.
Please drain apply work without holding the monitor needed by snapshot
creation and add a regression test for this interleaving.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
