[fix][broker] Avoid compaction task stuck when the last message to compact is a marker by coderzc · Pull Request #21718 · apache/pulsar

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:

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

            }

-            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

                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


+
+    @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

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

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 BewareMyPower, Demogorgon314, codelipenghui, congbobo184, gaoran10 and liangyepianzhou 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 Demogorgon314 and liangyepianzhou 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

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 merged 5 commits into
apache:masterfrom
coderzc:fix_compaction_marker

coderzc commented Dec 13, 2023 •

edited

Loading

Uh oh!

Uh oh!

Technoboy- commented Dec 14, 2023

Uh oh!

coderzc commented Dec 14, 2023

Uh oh!

codecov-commenter commented Dec 15, 2023

Uh oh!

liangyepianzhou Dec 18, 2023

Uh oh!

coderzc Dec 18, 2023

Uh oh!

Demogorgon314 Dec 19, 2023

Uh oh!

coderzc Dec 19, 2023

Uh oh!

Demogorgon314 Dec 19, 2023

Uh oh!

coderzc Dec 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants



		@Test
		public void testReadCommittedWithCompaction() throws Exception{

Conversation

coderzc commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

Uh oh!

Technoboy- commented Dec 14, 2023

Uh oh!

coderzc commented Dec 14, 2023

Uh oh!

codecov-commenter commented Dec 15, 2023

Codecov Report

Uh oh!

liangyepianzhou Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

coderzc Dec 18, 2023

Choose a reason for hiding this comment

Uh oh!

Demogorgon314 Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

coderzc Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

Demogorgon314 Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

coderzc Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coderzc commented Dec 13, 2023 •

edited

Loading