[fix][broker] Fix the markdelete position does not move forward when isAutoSkipNonRecoverableData=true and individual ack by lordcheng10 · Pull Request #17751 · apache/pulsar

lordcheng10 · 2022-09-20T16:44:03Z

Motivation

When isAutoSkipNonRecoverableData=true and individual ack, the markdelete position does not move forward.

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java

Lines 100 to 114 in 72dd01b

    
           } else if (cursor.config.isAutoSkipNonRecoverableData() && exception instanceof NonRecoverableLedgerException) { 
        
               log.warn("[{}][{}] read failed from ledger at position:{} : {}", cursor.ledger.getName(), cursor.getName(), 
        
                       readPosition, exception.getMessage()); 
        
               // try to find and move to next valid ledger 
        
               final Position nexReadPosition = cursor.getNextLedgerPosition(readPosition.getLedgerId()); 
        
               // fail callback if it couldn't find next valid ledger 
        
               if (nexReadPosition == null) { 
        
                   callback.readEntriesFailed(exception, ctx); 
        
                   cursor.ledger.mbean.recordReadEntriesError(); 
        
                   recycle(); 
        
                   return; 
        
               } 
        
               updateReadPosition(nexReadPosition); 
        
               checkReadCompletion(); 
        
           } else {

Modifications

Add a skipped variable in EntryImpl to record whether the entry is automatically skipped;
When pushing entrys to the consumer, automatically ack the skipped entrys;

Documentation

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

Matching PR in forked repository

PR in forked repository: lordcheng10#19

lordcheng10 · 2022-09-20T17:20:57Z

@eolivelli @codelipenghui @Technoboy- PTAL,thanks!

eolivelli · 2022-09-25T12:44:31Z

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

+     */
+    public List<Entry> filterAndAcknowledgeSkippedEntry(List<Entry> entries) {
+        List<Position> skippedPositions = new ArrayList<>();
+        List<Entry> filterEntries = Lists.newArrayList(Collections2.filter(entries, entry -> {


I wonder if we can move this into filterEntriesForConsumer? This way we save some allocations

Fixed!
@eolivelli PTAL,thanks!

eolivelli · 2022-09-27T07:18:16Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java

+        entry.ledgerId = ledgerId;
+        entry.entryId = entryId;
+        entry.skipped = skipped;
+        entry.data = Unpooled.wrappedBuffer(new byte[0]);


do we really need to allocate this ? what about using a constant ?

entry.data = Unpooled.EMPTY_BUFFER ?

Fixed! PTAL,thanks! @eolivelli

eolivelli · 2022-09-27T07:19:34Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java

        return entry;
    }

+    public static EntryImpl create(long ledgerId, long entryId, boolean skipped) {


what about "createSkippedEntry" ?
otherwise people may want to use this factory method for other purposes

Fixed! PTAL,thanks! @eolivelli

eolivelli

LGTM

lordcheng10 · 2022-09-27T12:00:48Z

@codelipenghui @Technoboy- @Jason918 PTAL,thanks!

Jason918 · 2022-09-27T13:04:51Z

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

+                    if (entriesToFiltered == null) {
+                        entriesToFiltered = new ArrayList<>();
+                    }
+                    entriesToFiltered.add(entryImpl.getPosition());


Do we need entries.set(i, null); and entry.release();?

OK , I will fix

Fixed, PTAL, thanks! @Jason918

Jason918 · 2022-09-27T13:06:43Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java

+            PositionImpl endPosition = (PositionImpl) nexReadPosition;
+            while (startPosition.compareTo(endPosition) < 0) {
+                skippedEntries.add(EntryImpl.createSkippedEntry(startPosition.ledgerId, startPosition.entryId));
+                startPosition = ledger.getNextValidPosition(startPosition);


It seems, normally, it will just goes from readPosition to nexReadPosition?
Will you miss other entries to be acked?

It seems, normally, it will just goes from readPosition to nexReadPosition?

YES

Will you miss other entries to be acked?

IMO, won't miss other entries to be acked

lhotari · 2024-10-14T12:17:17Z

It seems that #21210 addresses the same issue.

lordcheng10 changed the title ~~When isAutoSkipNonRecoverableData=true, fix the problem that the mark…~~ [fix][broker] Fix the problem that the markdelete position does not move forward Sep 20, 2022

lordcheng10 changed the title ~~[fix][broker] Fix the problem that the markdelete position does not move forward~~ [fix][broker] Fix the markdelete position does not move forward when isAutoSkipNonRecoverableData=true Sep 20, 2022

lordcheng10 changed the title ~~[fix][broker] Fix the markdelete position does not move forward when isAutoSkipNonRecoverableData=true~~ [fix][broker] Fix the markdelete position does not move forward when isAutoSkipNonRecoverableData=true and individual ack Sep 20, 2022

lordcheng10 mentioned this pull request Sep 20, 2022

[run-tests]Fix the markdelete position does not move forward when isAutoSkipNonRecoverableData=true and individual ack lordcheng10/pulsar#7

Closed

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 20, 2022

lordcheng10 force-pushed the fix_skip_entry_no_ack branch from d1ee2ee to 2f52eb1 Compare September 21, 2022 14:11

lordcheng10 mentioned this pull request Sep 21, 2022

[run-tests] [fix][broker] Fix the markdelete position does not move forward when isAutoSkipNonRecoverableData=true and individual ack lordcheng10/pulsar#14

Closed

lordcheng10 force-pushed the fix_skip_entry_no_ack branch from 2f52eb1 to 253273c Compare September 25, 2022 07:01

lordcheng10 mentioned this pull request Sep 25, 2022

[run-tests] [fix][broker] Fix the markdelete position does not move forward when isAutoSkipNonRecoverableData=true and individual ack lordcheng10/pulsar#19

Open

eolivelli requested changes Sep 25, 2022

View reviewed changes

lordcheng10 requested a review from eolivelli September 25, 2022 13:49

eolivelli requested changes Sep 27, 2022

View reviewed changes

lordcheng10 requested a review from eolivelli September 27, 2022 10:30

eolivelli approved these changes Sep 27, 2022

View reviewed changes

lordcheng10 force-pushed the fix_skip_entry_no_ack branch from 5b0c3a2 to e835c9f Compare September 27, 2022 11:41

Jason918 requested a review from AnonHxy September 27, 2022 12:39

Jason918 assigned lordcheng10 Sep 27, 2022

Jason918 added release/2.9.4 release/2.8.5 release/2.11.1 release/2.10.3 type/bug The PR fixed a bug or issue reported a bug area/broker labels Sep 27, 2022

Jason918 reviewed Sep 27, 2022

View reviewed changes

lordcheng10 requested review from Jason918 and removed request for AnonHxy September 27, 2022 13:43

github-actions bot removed the Stale label Jun 28, 2023

Technoboy- added this to the 3.2.0 milestone Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

lhotari added the release/4.0.1 label Oct 14, 2024

lhotari added release/4.0.2 and removed release/4.0.1 labels Dec 6, 2024

lhotari added release/4.0.3 and removed release/4.0.2 labels Jan 6, 2025

lhotari added release/4.0.4 and removed release/4.0.3 labels Feb 19, 2025

lhotari added release/4.0.5 and removed release/4.0.4 labels Mar 31, 2025

lhotari added release/4.0.6 and removed release/4.0.5 labels May 21, 2025

lhotari added release/4.0.7 and removed release/4.0.6 labels Jul 30, 2025

coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025

lhotari added release/4.0.8 and removed release/4.0.7 labels Sep 27, 2025

lhotari added release/4.0.9 and removed release/4.0.8 labels Nov 11, 2025

lhotari added release/4.0.10 and removed release/4.0.9 labels Feb 9, 2026

	} else if (cursor.config.isAutoSkipNonRecoverableData() && exception instanceof NonRecoverableLedgerException) {
	log.warn("[{}][{}] read failed from ledger at position:{} : {}", cursor.ledger.getName(), cursor.getName(),
	readPosition, exception.getMessage());
	// try to find and move to next valid ledger
	final Position nexReadPosition = cursor.getNextLedgerPosition(readPosition.getLedgerId());
	// fail callback if it couldn't find next valid ledger
	if (nexReadPosition == null) {
	callback.readEntriesFailed(exception, ctx);
	cursor.ledger.mbean.recordReadEntriesError();
	recycle();
	return;
	}
	updateReadPosition(nexReadPosition);
	checkReadCompletion();
	} else {

Conversation

lordcheng10 commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

lordcheng10 commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Sep 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari commented Oct 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

lordcheng10 commented Sep 20, 2022 •

edited

Loading

lordcheng10 Sep 27, 2022 •

edited

Loading

lordcheng10 Sep 27, 2022 •

edited

Loading