apurtell commented on code in PR #2596:
URL: https://github.com/apache/phoenix/pull/2596#discussion_r3786921338
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLog.java:
##########
@@ -251,19 +251,62 @@ protected void checkAndReplaceWriter(boolean asyncClose) {
* current writer stays open so in-flight writes still land. Skipping ahead
of the CAS (rather
* than inside {@link LogRotationTask#run()}) keeps the gate clear, so a
later tick resumes
* rotation as soon as the flag clears on abort.
+ * @return {@code true} if a rotation is now queued or already in flight
(worth waiting for);
+ * {@code false} if rotation is suppressed this call (failover
pending, or the executor is
+ * shutting down) so no task will run.
*/
- private void requestRotation() {
+ private boolean requestRotation() {
if (logGroup.isFailoverPending()) {
LOG.info("HAGroup {} rotation suspended: failover pending", logGroup);
- return;
+ return false;
}
if (rotationRequested.compareAndSet(false, true)) {
try {
rotationExecutor.execute(new LogRotationTask());
} catch (java.util.concurrent.RejectedExecutionException e) {
LOG.info("Rotation executor shut down, skipping rotation", e);
rotationRequested.set(false);
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Requests rotation and waits, bounded by {@code retryDelayMs}, for {@link
LogRotationTask} to
+ * stage a fresh writer in {@code pendingWriter}. Called only from {@link
#apply}'s retry path so
+ * a failed write is retried on a brand-new writer (new HDFS pipeline),
never the fenced one.
+ * <p>
+ * Each spin re-issues {@link #requestRotation()} before waiting: a request
coalesced away while a
+ * soon-to-complete rotation held the CAS gate is reissued once the gate
clears, so a fresh task
+ * actually gets scheduled. The waited-on condition is {@code pendingWriter}
itself, so a spurious
+ * or unrelated notify just re-checks and loops. Exits immediately when
rotation is permanently
+ * suppressed (nothing will ever stage). A close is observed on the next
wakeup rather than
+ * promptly — {@link #close} does not notify {@code rotationSignal} — but
the wait is bounded by
Review Comment:
`close()` could signal `rotationSignal`, e.g.
```java
public void close(boolean graceful) {
if (!closed.compareAndSet(false, true)) { return; }
synchronized (rotationSignal) { rotationSignal.notifyAll(); } // <--- HERE
stopRotationExecutor();
...
}
```
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLog.java:
##########
@@ -251,19 +251,62 @@ protected void checkAndReplaceWriter(boolean asyncClose) {
* current writer stays open so in-flight writes still land. Skipping ahead
of the CAS (rather
* than inside {@link LogRotationTask#run()}) keeps the gate clear, so a
later tick resumes
* rotation as soon as the flag clears on abort.
+ * @return {@code true} if a rotation is now queued or already in flight
(worth waiting for);
+ * {@code false} if rotation is suppressed this call (failover
pending, or the executor is
+ * shutting down) so no task will run.
*/
- private void requestRotation() {
+ private boolean requestRotation() {
if (logGroup.isFailoverPending()) {
LOG.info("HAGroup {} rotation suspended: failover pending", logGroup);
- return;
+ return false;
}
if (rotationRequested.compareAndSet(false, true)) {
try {
rotationExecutor.execute(new LogRotationTask());
} catch (java.util.concurrent.RejectedExecutionException e) {
LOG.info("Rotation executor shut down, skipping rotation", e);
rotationRequested.set(false);
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Requests rotation and waits, bounded by {@code retryDelayMs}, for {@link
LogRotationTask} to
+ * stage a fresh writer in {@code pendingWriter}. Called only from {@link
#apply}'s retry path so
+ * a failed write is retried on a brand-new writer (new HDFS pipeline),
never the fenced one.
+ * <p>
+ * Each spin re-issues {@link #requestRotation()} before waiting: a request
coalesced away while a
+ * soon-to-complete rotation held the CAS gate is reissued once the gate
clears, so a fresh task
+ * actually gets scheduled. The waited-on condition is {@code pendingWriter}
itself, so a spurious
+ * or unrelated notify just re-checks and loops. Exits immediately when
rotation is permanently
+ * suppressed (nothing will ever stage). A close is observed on the next
wakeup rather than
+ * promptly — {@link #close} does not notify {@code rotationSignal} — but
the wait is bounded by
+ * {@code retryDelayMs}, so a waiter unwinds within that budget regardless.
+ * @return the staged writer, or {@code null} if none was staged before the
deadline / close /
+ * permanent suppression. The caller drains it via {@link
#checkAndReplaceWriter}.
+ */
+ private LogFileWriter awaitStagedWriter() throws InterruptedIOException {
Review Comment:
If a rotation fails fast the loop can spin through all rotations in
milliseconds and close the log before the SAF downgrade even gets a chance to
help. Consider a tiny back-off e.g. `min(remainingMs, 50)` on the first spin,
or a fraction of `retryDelayMs` between spins.
--
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]
