Skip to content

[fix][txn] Fix PendingAckHandleImpl when pendingAckStoreProvider.checkInitializedBefore failed#18859

Merged
Technoboy- merged 2 commits intoapache:masterfrom
tjiuming:fix/pending_ack_handle
Dec 12, 2022
Merged

[fix][txn] Fix PendingAckHandleImpl when pendingAckStoreProvider.checkInitializedBefore failed#18859
Technoboy- merged 2 commits intoapache:masterfrom
tjiuming:fix/pending_ack_handle

Conversation

@tjiuming
Copy link
Contributor

@tjiuming tjiuming commented Dec 10, 2022

Motivation

When initialize PendingAckHandleImpl, it will call

        pendingAckStoreProvider.checkInitializedBefore(persistentSubscription)
                .thenAccept(init -> {
                    if (init) {
                        initPendingAckStore();
                    } else {
                        completeHandleFuture();
                    }
                });

And pendingAckStoreProvider.checkInitializedBefore(persistentSubscription) will call MetadataStore#asyncExists(String ledgerName)

If MetadataStore no available at that time, it may returns an exception.
If we don't handle the exception when call

pendingAckStoreProvider.checkInitializedBefore(persistentSubscription)

the PendingAckHandleImpl cannot completed.
addConsumer will always block, the Consumer created in PersistentTopic will never close and finished.

Documentation

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

Matching PR in forked repository

PR in forked repository: tjiuming#14

@github-actions
Copy link

@tjiuming Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! when PendingAckHandle recovery fails, it will not recover again, the consumer still can't connect to the broker. we should find out a way to solve this problem, if recovery fails, remove the subscription from PersistentTopic, and consumer connect again will reinit this sub.

@tjiuming
Copy link
Contributor Author

tjiuming commented Dec 10, 2022

Good catch! when PendingAckHandle recovery fails, it will not recover again, the consumer still can't connect to the broker. we should find out a way to solve this problem, if recovery fails, remove the subscription from PersistentTopic, and consumer connect again will reinit this sub.

Yes, it does a problem.
I've read the code, if PendingAckHandleImpl relay failed, it won't do it again. And the client couldn't create Consumer for the subscription.
I'll create another PR to fix it.

@Technoboy- Technoboy- assigned tjiuming and unassigned congbobo184 Dec 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2022

Codecov Report

Merging #18859 (955e1fe) into master (d30e86c) will increase coverage by 0.01%.
The diff coverage is 31.25%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18859      +/-   ##
============================================
+ Coverage     47.92%   47.93%   +0.01%     
- Complexity    10679    10688       +9     
============================================
  Files           703      703              
  Lines         68831    68856      +25     
  Branches       7378     7382       +4     
============================================
+ Hits          32988    33009      +21     
- Misses        32119    32177      +58     
+ Partials       3724     3670      -54     
Flag Coverage Δ
unittests 47.93% <31.25%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...er/loadbalance/impl/PulsarResourceDescription.java 11.11% <0.00%> (-1.71%) ⬇️
...nsaction/pendingack/impl/PendingAckHandleImpl.java 51.73% <50.00%> (-0.11%) ⬇️
...ain/java/org/apache/pulsar/broker/rest/Topics.java 0.00% <0.00%> (-100.00%) ⬇️
.../pulsar/broker/rest/RestMessagePublishContext.java 0.00% <0.00%> (-77.78%) ⬇️
...ker/resourcegroup/ResourceQuotaCalculatorImpl.java 4.16% <0.00%> (-62.50%) ⬇️
...r/resourcegroup/ResourceUsageTransportManager.java 42.85% <0.00%> (-57.15%) ⬇️
...lsar/broker/service/StickyKeyConsumerSelector.java 50.00% <0.00%> (-50.00%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 0.00% <0.00%> (-50.00%) ⬇️
...java/org/apache/pulsar/broker/rest/TopicsBase.java 0.00% <0.00%> (-40.67%) ⬇️
...e/HashRangeExclusiveStickyKeyConsumerSelector.java 0.00% <0.00%> (-40.00%) ⬇️
... and 78 more

@Technoboy- Technoboy- merged commit 1be5a69 into apache:master Dec 12, 2022
@Technoboy- Technoboy- added this to the 2.12.0 milestone Dec 12, 2022
Technoboy- pushed a commit that referenced this pull request Dec 12, 2022
liangyepianzhou pushed a commit that referenced this pull request Dec 14, 2022
…ckInitializedBefore` failed (#18859)

(cherry picked from commit 1be5a69)
congbobo184 pushed a commit that referenced this pull request Dec 16, 2022
…ckInitializedBefore` failed (#18859)

(cherry picked from commit 1be5a69)
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 16, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
…ckInitializedBefore` failed (apache#18859)

(cherry picked from commit 1be5a69)
(cherry picked from commit e5dedfe)
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
…ckInitializedBefore` failed (apache#18859)

(cherry picked from commit 1be5a69)
(cherry picked from commit e5dedfe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants