Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Broker] Fix messageDedup delete inactive producer name #12493

Conversation

congbobo184
Copy link
Contributor

Motivation

fix #12478

Implement

when message deup recover, put producerName into inactiveProduceName map, if producer connect to, it will be removed from inactiveProduceName map

Verifying this change

Add the tests for it

Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (yes)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

@eolivelli
Copy link
Contributor

@congbobo184:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@eolivelli
Copy link
Contributor

@congbobo184:Thanks for providing doc info!

@congbobo184 congbobo184 added this to the 2.10.0 milestone Oct 26, 2021
Copy link
Contributor

@315157973 315157973 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -443,15 +445,15 @@ public void markDeleteFailed(ManagedLedgerException exception, Object ctx) {
/**
* Topic will call this method whenever a producer connects.
*/
public synchronized void producerAdded(String producerName) {
public void producerAdded(String producerName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't remove the synchronized here because it's used also to make operations atomic with other methods. eg. purgeInactiveProducers()

@congbobo184 congbobo184 merged commit 928924b into apache:master Oct 27, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Oct 27, 2021
* up/master:
  fix delete authentication policies when delete topic. (apache#12215)
  [Broker] Fix messageDedup delete inactive producer name (apache#12493)
  Fixed getting children of parent nodes in LocalMemoryMetadataStore (apache#12491)
  Update Producer stats on producer close() (apache#12500)
  docs(cli):add restart command in pulsar-daemon (apache#12373)
  Add the pulsar java property memory allocator doc (apache#12481)
  [Doc]Update ci-documentbot.yml (apache#12480)
@merlimat merlimat added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Oct 30, 2021
merlimat pushed a commit that referenced this pull request Oct 30, 2021
Now, remove inactive producerName in MessageDeduplication when producer close. But the producer has been closed before topic unload, this producerName will not be remove if producer don't connect broker with the same producerName.

When topic recover `MessageDeduplication`, we should put every producerName into inactive producerNameMap. When producer with the same name, we will remove it from the inactive map, if this producerName can not connect within brokerDeduplicationProducerInactivityTimeoutMinutes, we can remove it.
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
## Issue
Now, remove inactive producerName in MessageDeduplication when producer close. But the producer has been closed before topic unload, this producerName will not be remove if producer don't connect broker with the same producerName.

## implement

When topic recover `MessageDeduplication`, we should put every producerName into inactive producerNameMap. When producer with the same name, we will remove it from the inactive map, if this producerName can not connect within brokerDeduplicationProducerInactivityTimeoutMinutes, we can remove it.
@lhotari lhotari added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 9, 2021
lhotari pushed a commit that referenced this pull request Dec 9, 2021
## Issue
Now, remove inactive producerName in MessageDeduplication when producer close. But the producer has been closed before topic unload, this producerName will not be remove if producer don't connect broker with the same producerName.

## implement

When topic recover `MessageDeduplication`, we should put every producerName into inactive producerNameMap. When producer with the same name, we will remove it from the inactive map, if this producerName can not connect within brokerDeduplicationProducerInactivityTimeoutMinutes, we can remove it.

(cherry picked from commit 928924b)
lhotari pushed a commit to datastax/pulsar that referenced this pull request Dec 10, 2021
Now, remove inactive producerName in MessageDeduplication when producer close. But the producer has been closed before topic unload, this producerName will not be remove if producer don't connect broker with the same producerName.

When topic recover `MessageDeduplication`, we should put every producerName into inactive producerNameMap. When producer with the same name, we will remove it from the inactive map, if this producerName can not connect within brokerDeduplicationProducerInactivityTimeoutMinutes, we can remove it.

(cherry picked from commit 04e8d7e)
Jason918 pushed a commit that referenced this pull request Jul 27, 2022
Now, remove inactive producerName in MessageDeduplication when producer close. But the producer has been closed before topic unload, this producerName will not be remove if producer don't connect broker with the same producerName.

When topic recover `MessageDeduplication`, we should put every producerName into inactive producerNameMap. When producer with the same name, we will remove it from the inactive map, if this producerName can not connect within brokerDeduplicationProducerInactivityTimeoutMinutes, we can remove it.

(cherry picked from commit 928924b)
@Jason918 Jason918 added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.7.5 release/2.8.2 release/2.9.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Broker] MessageDeduplication with producerName can not be cleaned up
7 participants