[fix][broker] Remove failed OpAddEntry from pendingAddEntries #23817

gaoran10 · 2025-01-06T15:12:17Z

Motivation

The PR cached the exceptions for entry payload interceptor processor, but the failed OpAddEntry wasn't removed from pendingAddEntries, this will cause the next write to fail.

Modifications

Remove failed OpAddEntry from pendingAddEntries.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

dao-jun · 2025-01-07T05:54:35Z

Does it will lead to msg out of order?

gaoran10 · 2025-01-07T06:59:09Z

Does it will lead to msg out of order?

No, because the failed OpAddEntry wasn't removed from the pendingAddEntries of ManagedLedgerImpl, the subsequent write operations will encounter the ManagedLedgerException "Unexpected add entry op when complete the add entry op.", refer to here.

(cherry picked from commit 420f62e)

lhotari · 2025-01-07T17:30:37Z

@gaoran10 The after cherry-picking to branch-3.0 and branch-3.3, the test fails.

[ERROR] Tests run: 14, Failures: 1, Errors: 0, Skipped: 9, Time elapsed: 1.293 s <<< FAILURE! - in org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImplTest
[ERROR] org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImplTest.testManagedLedgerPayloadInputProcessorFailure  Time elapsed: 0.019 s  <<< FAILURE!
java.lang.AssertionError: expected [5] but found [10]
        at org.testng.Assert.fail(Assert.java:110)
        at org.testng.Assert.failNotEquals(Assert.java:1577)
        at org.testng.Assert.assertEqualsImpl(Assert.java:149)
        at org.testng.Assert.assertEquals(Assert.java:131)
        at org.testng.Assert.assertEquals(Assert.java:1418)
        at org.testng.Assert.assertEquals(Assert.java:1382)
        at org.testng.Assert.assertEquals(Assert.java:1428)
        at org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImplTest.testManagedLedgerPayloadInputProcessorFailure(ManagedLedgerInterceptorImplTest.java:500)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:569)
        at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
        at org.testng.internal.invokers.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:47)
        at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:76)
        at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:840)

In branch-4.0, there wasn't a problem in applying the changes. Can you please handle cherry-picking and backporting to branch-3.0 and branch-3.3?

lhotari · 2025-01-07T17:41:26Z

...roker/src/test/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImplTest.java

@@ -446,26 +446,33 @@ public Processor inputProcessor() {
                return new Processor() {
                    @Override
                    public ByteBuf process(Object contextObj, ByteBuf inputPayload) {
-                        throw new RuntimeException(failureMsg);
+                        Commands.skipBrokerEntryMetadataIfExist(inputPayload);


In branch-3.0 & branch-3.3, the test fails due to this line. Removing it makes the test pass.

java.lang.IndexOutOfBoundsException: readerIndex(0) + length(2) exceeds writerIndex(1): UnpooledDuplicatedByteBuf(ridx: 0, widx: 1, cap: 256, unwrapped: UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 1, cap: 256)) at io.netty.buffer.AbstractByteBuf.checkReadableBytes0(AbstractByteBuf.java:1442) ~[netty-buffer-4.1.116.Final.jar:4.1.116.Final] at io.netty.buffer.AbstractByteBuf.readShort(AbstractByteBuf.java:749) ~[netty-buffer-4.1.116.Final.jar:4.1.116.Final] at org.apache.pulsar.common.protocol.Commands.skipBrokerEntryMetadataIfExist(Commands.java:1706) ~[classes/:?] at org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImplTest$3$1.process(ManagedLedgerInterceptorImplTest.java:454) ~[test-classes/:?] at org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImpl.processPayload(ManagedLedgerInterceptorImpl.java:171) ~[classes/:?] at org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImpl.processPayloadBeforeLedgerWrite(ManagedLedgerInterceptorImpl.java:196) ~[classes/:?] at org.apache.bookkeeper.mledger.impl.OpAddEntry.initiate(OpAddEntry.java:143) ~[classes/:?] at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.internalAsyncAddEntry(ManagedLedgerImpl.java:873) ~[classes/:?] at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl.lambda$asyncAddEntry$2(ManagedLedgerImpl.java:787) ~[classes/:?] at org.apache.bookkeeper.common.util.SingleThreadSafeScheduledExecutorService$SafeRunnable.run(SingleThreadSafeScheduledExecutorService.java:46) ~[bookkeeper-common-4.16.6.jar:4.16.6] at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?] at java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:264) ~[?:?] at java.util.concurrent.FutureTask.run(FutureTask.java) ~[?:?] at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?] at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?] at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.116.Final.jar:4.1.116.Final] at java.lang.Thread.run(Thread.java:840) ~[?:?]

@gaoran10 I'll remove this line while backporting to branch-3.0 and branch-3.3

Oh, thanks.

(cherry picked from commit 420f62e)

lhotari · 2025-01-07T17:49:37Z

...roker/src/test/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImplTest.java

        var expectedException = new ArrayList<Exception>();
-        ledger.asyncAddEntry("test".getBytes(), 1, 1, new AsyncCallbacks.AddEntryCallback() {
+
+        var addEntryCallback = new AsyncCallbacks.AddEntryCallback() {
            @Override
            public void addComplete(Position position, ByteBuf entryData, Object ctx) {
                entryData.release();


in the original test, calling entryData.release() seems to be invalid since there are warning logs:

2025-01-07T19:48:06,671 - WARN - [test-OrderedScheduler-0-0:SingleThreadSafeScheduledExecutorService] - Unexpected throwable from task class org.apache.bookkeeper.mledger.impl.OpAddEntry: refCnt: 0, decrement: 1 io.netty.util.IllegalReferenceCountException: refCnt: 0, decrement: 1 at io.netty.util.internal.ReferenceCountUpdater.toLiveRealRefCnt(ReferenceCountUpdater.java:83) ~[netty-common-4.1.116.Final.jar:4.1.116.Final] at io.netty.util.internal.ReferenceCountUpdater.release(ReferenceCountUpdater.java:148) ~[netty-common-4.1.116.Final.jar:4.1.116.Final] at io.netty.buffer.AbstractReferenceCountedByteBuf.release(AbstractReferenceCountedByteBuf.java:101) ~[netty-buffer-4.1.116.Final.jar:4.1.116.Final] at io.netty.util.ReferenceCountUtil.release(ReferenceCountUtil.java:90) ~[netty-common-4.1.116.Final.jar:4.1.116.Final] at org.apache.bookkeeper.mledger.impl.OpAddEntry.run(OpAddEntry.java:277) ~[classes/:?] at org.apache.bookkeeper.common.util.SingleThreadSafeScheduledExecutorService$SafeRunnable.run(SingleThreadSafeScheduledExecutorService.java:46) ~[bookkeeper-common-4.17.1.jar:4.17.1] at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572) ~[?:?] at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:317) ~[?:?] at java.base/java.util.concurrent.FutureTask.run(FutureTask.java) ~[?:?] at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?] at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?] at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?] at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[netty-common-4.1.116.Final.jar:4.1.116.Final] at java.base/java.lang.Thread.run(Thread.java:1583) [?:?]

Oh, sorry, we don't need to release the ByteBuf here. I'll fix the test. Thanks.

…#23817) (cherry picked from commit 420f62e) (cherry picked from commit 0a0b3ca)

remove failed OpAddEntry from pendingAddEntries

dcbdeb9

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 6, 2025

gaoran10 requested review from lhotari, nodece, BewareMyPower, dao-jun and poorbarcode January 6, 2025 15:13

gaoran10 self-assigned this Jan 6, 2025

gaoran10 closed this Jan 6, 2025

gaoran10 reopened this Jan 6, 2025

gaoran10 requested a review from Technoboy- January 6, 2025 15:18

lhotari added this to the 4.1.0 milestone Jan 7, 2025

lhotari added release/3.0.9 release/3.3.4 release/4.0.2 labels Jan 7, 2025

lhotari approved these changes Jan 7, 2025

View reviewed changes

nodece approved these changes Jan 7, 2025

View reviewed changes

dao-jun approved these changes Jan 7, 2025

View reviewed changes

lhotari merged commit 420f62e into apache:master Jan 7, 2025
77 of 81 checks passed

lhotari pushed a commit that referenced this pull request Jan 7, 2025

[fix][broker] Remove failed OpAddEntry from pendingAddEntries (#23817)

ef15ce3

(cherry picked from commit 420f62e)

lhotari added the cherry-picked/branch-4.0 label Jan 7, 2025

lhotari reviewed Jan 7, 2025

View reviewed changes

lhotari pushed a commit that referenced this pull request Jan 7, 2025

[fix][broker] Remove failed OpAddEntry from pendingAddEntries (#23817)

0a0b3ca

(cherry picked from commit 420f62e)

lhotari added the cherry-picked/branch-3.0 label Jan 7, 2025

lhotari pushed a commit that referenced this pull request Jan 7, 2025

[fix][broker] Remove failed OpAddEntry from pendingAddEntries (#23817)

41c7e29

(cherry picked from commit 420f62e)

lhotari added the cherry-picked/branch-3.3 label Jan 7, 2025

lhotari reviewed Jan 7, 2025

View reviewed changes

gaoran10 mentioned this pull request Jan 8, 2025

[fix][test] Remove useless test code #23823

Merged

4 tasks

nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2025

[fix][broker] Remove failed OpAddEntry from pendingAddEntries (apache…

2526c8f

…#23817) (cherry picked from commit 420f62e) (cherry picked from commit 0a0b3ca)

srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jan 13, 2025

[fix][broker] Remove failed OpAddEntry from pendingAddEntries (apache…

7489004

…#23817) (cherry picked from commit 420f62e) (cherry picked from commit 0a0b3ca)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Remove failed OpAddEntry from pendingAddEntries #23817

[fix][broker] Remove failed OpAddEntry from pendingAddEntries #23817

gaoran10 commented Jan 6, 2025

dao-jun commented Jan 7, 2025

gaoran10 commented Jan 7, 2025 •

edited

Loading

lhotari commented Jan 7, 2025

lhotari Jan 7, 2025

lhotari Jan 7, 2025

lhotari Jan 7, 2025

gaoran10 Jan 8, 2025

lhotari Jan 7, 2025

gaoran10 Jan 8, 2025 •

edited

Loading

gaoran10 Jan 8, 2025

[fix][broker] Remove failed OpAddEntry from pendingAddEntries #23817

[fix][broker] Remove failed OpAddEntry from pendingAddEntries #23817

Conversation

gaoran10 commented Jan 6, 2025

Motivation

Modifications

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

dao-jun commented Jan 7, 2025

gaoran10 commented Jan 7, 2025 • edited Loading

lhotari commented Jan 7, 2025

lhotari Jan 7, 2025

Choose a reason for hiding this comment

lhotari Jan 7, 2025

Choose a reason for hiding this comment

lhotari Jan 7, 2025

Choose a reason for hiding this comment

gaoran10 Jan 8, 2025

Choose a reason for hiding this comment

lhotari Jan 7, 2025

Choose a reason for hiding this comment

gaoran10 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

gaoran10 Jan 8, 2025

Choose a reason for hiding this comment

gaoran10 commented Jan 7, 2025 •

edited

Loading

gaoran10 Jan 8, 2025 •

edited

Loading