apurtell commented on code in PR #2485:
URL: https://github.com/apache/phoenix/pull/2485#discussion_r3277776306
##########
phoenix-core/src/test/java/org/apache/phoenix/replication/ReplicationLogGroupTest.java:
##########
@@ -1907,4 +1908,73 @@ public void
testBlockFullSyncOnAppendReducesReplayOnRotation() throws Exception
assertTrue("Replay should be bounded by last partial block, not all
records. Got: "
+ newWriterAppendCount, newWriterAppendCount < id);
}
+
+ /**
+ * Tests that the size check is not invoked from inside apply(). Pre-fix,
every successful action
+ * inside apply() ran requestRotationIfNeeded(); when a rotation swapped in
a new writer mid-batch
+ * and replay made it exceed the threshold, the size check after every
append would request
+ * another rotation, drain it on the next apply(), replay again, and loop
indefinitely.
+ * <p>
+ * The new writer is mocked so its getLength() always reports above the
rotation threshold. On the
+ * buggy code this triggers a rotation request after every successful
append, which cascades into
+ * one writer per append. With the fix, requestRotationIfNeeded() runs only
after currentBatch is
+ * cleared, so the chain is bounded.
+ */
+ @Test
+ public void testSizeRotationDoesNotLoopOnReplay() throws Exception {
+ final String tableName = "TBLSRDLOR";
+ long commitId = 1L;
+ // Long enough that only the first scheduled tick can fire within the test
window.
+ final int roundDurationSeconds = 1;
+ final int numAppendsAfterTick = 20;
+
+ conf.setInt(PHOENIX_REPLICATION_ROUND_DURATION_SECONDS_KEY,
roundDurationSeconds);
+ recreateLogGroup();
+
+ ReplicationLog activeLog = logGroup.getActiveLog();
+ LogFileWriter initialWriter = activeLog.getWriter();
+ assertNotNull("Initial writer should not be null", initialWriter);
+
+ // Count writers created so we can detect a rotation chain. Each writer
created after the
+ // initial one is configured to report a length above the rotation
threshold so the size
+ // check (wherever it runs) sees an over-threshold writer.
+ AtomicInteger writersCreated = new AtomicInteger(1); // initial writer
already created
+ final long oversizedLength = TEST_ROTATION_SIZE_BYTES + 1;
+ doAnswer(invocation -> {
+ LogFileWriter w = (LogFileWriter) invocation.callRealMethod();
+ doReturn(oversizedLength).when(w).getLength();
+ writersCreated.incrementAndGet();
+ return w;
+ }).when(activeLog).createNewWriter();
+
+ // Append unsynced records so currentBatch is non-empty when the rotation
tick fires.
+ final Mutation put = LogFileTestUtil.newPut("row", 1, 1);
+ for (int i = 0; i < 5; i++) {
+ logGroup.append(tableName, commitId++, put);
+ }
+
+ // Wait for the time-based rotation tick to stage a fresh pendingWriter.
+ waitForRotationTick(roundDurationSeconds);
+
+ // Continue appending. On the buggy code, the first such append drains the
staged writer,
+ // replays the batch, then requestRotationIfNeeded() inside apply() fires
(length is over
+ // threshold) → requests another rotation. Subsequent appends drain that
one too, replay,
+ // request another, and so on — one new writer per append.
+ for (int i = 0; i < numAppendsAfterTick; i++) {
+ logGroup.append(tableName, commitId++, put);
+ // Small sleep to give the on-demand rotation task time to actually
create the writer
+ // before the next append drains it. This makes the chain observable in
the test.
+ Thread.sleep(20);
+ }
Review Comment:
This is a good suggestion. Or a Waiter#waitFor test pattern on such a
latch...
--
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]
