[fix][broker] Avoid compaction task stuck when the last message to compact is a marker #21718

coderzc · 2023-12-13T07:40:41Z

Motivation

If the last message to compact is a marker, then the compaction task will stuck until timeout fails. The root cause is that the marker has been filtered out by the dispatcher, and the compaction reader will not read the last message.

 WARN  - [broker-client-shared-internal-executor-18-1:PersistentTopic@3432] - [persistent://tnx/ns-prechecks/test_transaction_topic211ca13d-f8a4-4648-a7b9-bd2f679e0f3b] Compaction failure.
java.util.concurrent.CompletionException: org.apache.pulsar.common.util.FutureUtil$LowOverheadTimeoutException: Timeout
	at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:332) ~[?:?]
	at java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:347) ~[?:?]
	at java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:708) ~[?:?]
	at java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510) ~[?:?]
	at java.util.concurrent.CompletableFuture.completeExceptionally(CompletableFuture.java:2162) ~[?:?]
	at org.apache.pulsar.common.util.FutureUtil.lambda$addTimeoutHandling$9(FutureUtil.java:236) ~[classes/:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:264) ~[?:?]
	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.100.Final.jar:4.1.100.Final]
	at java.lang.Thread.run(Thread.java:833) ~[?:?]
Caused by: org.apache.pulsar.common.util.FutureUtil$LowOverheadTimeoutException: Timeout
	at org.apache.pulsar.compaction.TwoPhaseCompactor.phaseOneLoop(...)(Unknown Source) ~[classes/:?]

Modifications

For __compaction internal cursor, we don't need to filter out marker messages and delete marker messages during compaction.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

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:

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

Technoboy- · 2023-12-14T13:44:08Z

We need to cherry to 2.9 ~ 3.1 right ?

coderzc · 2023-12-14T14:29:13Z

We need to cherry to 2.9 ~ 3.1 right ?

Yes.

codecov-commenter · 2023-12-15T02:11:37Z

Codecov Report

Merging #21718 (bbef1e6) into master (50007c3) will decrease coverage by 0.17%.
Report is 10 commits behind head on master.
The diff coverage is 93.33%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #21718      +/-   ##
============================================
- Coverage     73.50%   73.33%   -0.17%     
+ Complexity    32818    32754      -64     
============================================
  Files          1893     1893              
  Lines        140721   140768      +47     
  Branches      15502    15512      +10     
============================================
- Hits         103432   103228     -204     
- Misses        29200    29427     +227     
- Partials       8089     8113      +24

Flag	Coverage Δ
inttests	`24.14% <40.00%> (-0.02%)`	⬇️
systests	`24.72% <6.66%> (-0.12%)`	⬇️
unittests	`72.62% <93.33%> (-0.16%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...rg/apache/pulsar/compaction/TwoPhaseCompactor.java	`74.59% <100.00%> (+0.64%)`	⬆️
.../pulsar/broker/service/AbstractBaseDispatcher.java	`93.84% <85.71%> (+0.03%)`	⬆️

... and 80 files with indirect coverage changes

liangyepianzhou · 2023-12-18T10:27:11Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java

@@ -192,7 +195,7 @@ public int filterEntriesForConsumer(@Nullable MessageMetadata[] metadataArray, i
                }
            }

-            if (msgMetadata == null || Markers.isServerOnlyMarker(msgMetadata)) {
+            if (msgMetadata == null || (Markers.isServerOnlyMarker(msgMetadata) && !Markers.isTxnMarker(msgMetadata))) {


I think we should pass all the markers and filter them at the client side when doing compaction.

Demogorgon314 · 2023-12-19T04:00:00Z

pulsar-broker/src/main/java/org/apache/pulsar/compaction/TwoPhaseCompactor.java

@@ -262,7 +266,10 @@ private void phaseTwoLoop(RawReader reader, MessageId to, Map<String, MessageId>
                MessageId id = m.getMessageId();
                Optional<RawMessage> messageToAdd = Optional.empty();
                mxBean.addCompactionReadOp(reader.getTopic(), m.getHeadersAndPayload().readableBytes());
-                if (RawBatchConverter.isReadableBatch(m)) {
+                MessageMetadata metadata = Commands.parseMessageMetadata(m.getHeadersAndPayload());
+                if (Markers.isTxnMarker(metadata)) {


Why only check the txn marker here? Should we use Markers.isServerOnlyMarker(metadata)?

Yes, I will fix it.

Demogorgon314 · 2023-12-19T04:01:51Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java

@@ -1849,4 +1850,62 @@ public void testReadCommittedWithReadCompacted() throws Exception{
        Assert.assertEquals(messages, List.of("V2", "V3"));
    }

+
+    @Test
+    public void testReadCommittedWithCompaction() throws Exception{


Should we add a test to send a replicated subscription snapshot marker and test the compaction works?

I add test testReplicatedSubscriptionWithCompaction, PLAT~

…mpact is a marker (#21718)

…mpact is a marker (apache#21718) (cherry picked from commit 1f99568) Conflicts: pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java Fixed duplicate namespace created in transactionTest

…mpact is a marker (#21718)

Avoid compaction task stuck when the last message is marker

b6b3ea7

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 13, 2023

coderzc added type/bug The PR fixed a bug or issue reported a bug area/broker labels Dec 13, 2023

coderzc added this to the 3.2.0 milestone Dec 13, 2023

Technoboy- reviewed Dec 14, 2023

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java Show resolved Hide resolved

Technoboy- assigned coderzc Dec 14, 2023

Improve test

bbef1e6

coderzc added release/2.10.6 release/3.0.3 release/2.11.4 release/3.1.3 labels Dec 14, 2023

Technoboy- added the ready-to-test label Dec 15, 2023

Technoboy- closed this Dec 15, 2023

Technoboy- reopened this Dec 15, 2023

Technoboy- approved these changes Dec 15, 2023

View reviewed changes

coderzc requested review from codelipenghui, congbobo184, liangyepianzhou, Demogorgon314, gaoran10 and BewareMyPower December 18, 2023 01:56

liangyepianzhou reviewed Dec 18, 2023

View reviewed changes

Filter all marker

9b2709e

Demogorgon314 reviewed Dec 19, 2023

View reviewed changes

Address comment

55d929c

coderzc force-pushed the fix_compaction_marker branch from 56ba96a to 55d929c Compare December 19, 2023 04:40

Add test & fix code

3ce2f31

coderzc force-pushed the fix_compaction_marker branch from c4965c0 to 3ce2f31 Compare December 19, 2023 08:18

coderzc requested review from liangyepianzhou and Demogorgon314 December 19, 2023 08:21

liangyepianzhou approved these changes Dec 19, 2023

View reviewed changes

Demogorgon314 approved these changes Dec 19, 2023

View reviewed changes

coderzc merged commit fc393f6 into apache:master Dec 20, 2023
48 of 49 checks passed

coderzc deleted the fix_compaction_marker branch December 20, 2023 01:39

Technoboy- pushed a commit that referenced this pull request Jan 3, 2024

[fix][broker] Avoid compaction task stuck when the last message to co…

1f99568

…mpact is a marker (#21718)

Technoboy- added the cherry-picked/branch-3.1 label Jan 3, 2024

Technoboy- added the cherry-picked/branch-3.0 label Jan 16, 2024

Technoboy- pushed a commit that referenced this pull request Jan 16, 2024

[fix][broker] Avoid compaction task stuck when the last message to co…

b66bfd3

…mpact is a marker (#21718)

coderzc added area/compaction area/transaction labels Jan 20, 2024

thetumbled mentioned this pull request Jan 23, 2024

[fix] [broker] reader stuck with marker message though hasMessageAvailable() return true #21951

Closed

15 tasks

lhotari added release/2.10.7 release/2.11.5 and removed release/2.10.6 release/2.11.4 labels Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Avoid compaction task stuck when the last message to compact is a marker #21718

[fix][broker] Avoid compaction task stuck when the last message to compact is a marker #21718

coderzc commented Dec 13, 2023 •

edited

Technoboy- commented Dec 14, 2023

coderzc commented Dec 14, 2023

codecov-commenter commented Dec 15, 2023

liangyepianzhou Dec 18, 2023

coderzc Dec 18, 2023

Demogorgon314 Dec 19, 2023

coderzc Dec 19, 2023

Demogorgon314 Dec 19, 2023

coderzc Dec 19, 2023

[fix][broker] Avoid compaction task stuck when the last message to compact is a marker #21718

[fix][broker] Avoid compaction task stuck when the last message to compact is a marker #21718

Conversation

coderzc commented Dec 13, 2023 • edited

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Technoboy- commented Dec 14, 2023

coderzc commented Dec 14, 2023

codecov-commenter commented Dec 15, 2023

Codecov Report

liangyepianzhou Dec 18, 2023

Choose a reason for hiding this comment

coderzc Dec 18, 2023

Choose a reason for hiding this comment

Demogorgon314 Dec 19, 2023

Choose a reason for hiding this comment

coderzc Dec 19, 2023

Choose a reason for hiding this comment

Demogorgon314 Dec 19, 2023

Choose a reason for hiding this comment

coderzc Dec 19, 2023

Choose a reason for hiding this comment

coderzc commented Dec 13, 2023 •

edited