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

[improve] [broker] Let the producer request success at the first time if the previous one is inactive #21220

Merged

Conversation

poorbarcode
Copy link
Contributor

Motivation

See #21155 (comment)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail. This failure will trigger the topic of cleaning up the inactive producers. However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Modifications

Make the initial request made on the new connection success.

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 21, 2023
@poorbarcode poorbarcode self-assigned this Sep 21, 2023
@poorbarcode poorbarcode added release/3.0.2 release/2.11.3 release/2.10.6 category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost labels Sep 21, 2023
@poorbarcode poorbarcode added this to the 3.2.0 milestone Sep 21, 2023
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Oct 22, 2023
@poorbarcode poorbarcode added the type/bug The PR fixed a bug or issue reported a bug label Oct 27, 2023
@poorbarcode poorbarcode force-pushed the improve/producer_success_at_first_time branch from 3709b39 to 8710eb5 Compare October 31, 2023 09:22
"Producer with name '" + newProducer.getProducerName()
+ "' is already connected to topic"));
} else {
return internalAddProducer(newProducer);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to add a comment here to explain why we should take a recursive call.

Copy link
Contributor

Choose a reason for hiding this comment

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

And it also should be BUG before, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better to add a comment here to explain why we should take a recursive call.

Done

And it also should be BUG before, right?

Yes, the issue is that the producer creation will fail if a same-name producer has already closed on the client side.

@poorbarcode poorbarcode closed this Nov 1, 2023
@poorbarcode poorbarcode reopened this Nov 1, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #21220 (f7aa64f) into master (bc84721) will increase coverage by 36.48%.
Report is 4 commits behind head on master.
The diff coverage is 75.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21220       +/-   ##
=============================================
+ Coverage     36.74%   73.23%   +36.48%     
- Complexity      375    32607    +32232     
=============================================
  Files          1713     1890      +177     
  Lines        130808   140428     +9620     
  Branches      14256    15434     +1178     
=============================================
+ Hits          48068   102841    +54773     
+ Misses        76375    29498    -46877     
- Partials       6365     8089     +1724     
Flag Coverage Δ
inttests 24.16% <20.00%> (-0.09%) ⬇️
systests 24.69% <20.00%> (-0.05%) ⬇️
unittests 72.52% <75.00%> (+40.66%) ⬆️

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

Files Coverage Δ
...rg/apache/pulsar/broker/service/AbstractTopic.java 87.14% <75.00%> (+28.25%) ⬆️

... and 1454 files with indirect coverage changes

@poorbarcode poorbarcode merged commit 40f94d5 into apache:master Nov 2, 2023
51 of 67 checks passed
nborisov pushed a commit to nborisov/pulsar that referenced this pull request Nov 13, 2023
… if the previous one is inactive (apache#21220)

### Motivation

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.


### Modifications

Make the initial request made on the new connection success.
Technoboy- pushed a commit that referenced this pull request Nov 23, 2023
… if the previous one is inactive (#21220)

### Motivation

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.


### Modifications

Make the initial request made on the new connection success.
@AnonHxy
Copy link
Contributor

AnonHxy commented Nov 28, 2023

There are conflicts when cherry pick this patch to branch-3.1, would you please help do it @poorbarcode

poorbarcode added a commit that referenced this pull request Nov 28, 2023
… if the previous one is inactive (#21220)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Make the initial request made on the new connection success.

(cherry picked from commit 711b621)
@poorbarcode
Copy link
Contributor Author

There are conflicts when cherry pick this patch to branch-3.1, would you please help do it @poorbarcode

Done

@poorbarcode poorbarcode deleted the improve/producer_success_at_first_time branch November 28, 2023 15:29
poorbarcode added a commit that referenced this pull request Nov 28, 2023
… if the previous one is inactive (#21220)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Make the initial request made on the new connection success.

(cherry picked from commit 40f94d5)
poorbarcode added a commit that referenced this pull request Nov 28, 2023
…rst time if the previous one is inactive (#21220)"

This reverts commit 918e746.
poorbarcode added a commit that referenced this pull request Nov 28, 2023
… if the previous one is inactive (#21220)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Make the initial request made on the new connection success.

(cherry picked from commit 40f94d5)
poorbarcode added a commit that referenced this pull request Nov 28, 2023
… if the previous one is inactive (#21220)

### Motivation

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

### Modifications

Make the initial request made on the new connection success.

(cherry picked from commit 40f94d5)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Dec 2, 2023
… if the previous one is inactive (apache#21220)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Make the initial request made on the new connection success.

(cherry picked from commit 40f94d5)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Dec 2, 2023
…rst time if the previous one is inactive (apache#21220)"

This reverts commit 918e746.
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Dec 2, 2023
… if the previous one is inactive (apache#21220)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Make the initial request made on the new connection success.

(cherry picked from commit 40f94d5)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
… if the previous one is inactive (apache#21220)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Make the initial request made on the new connection success.

(cherry picked from commit 711b621)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 20, 2023
… if the previous one is inactive (apache#21220)

If a producer establishes a new connection when it is reconnecting, while the previous connection is now inactive, the initial request made on the new connection will fail.  This failure will trigger the topic of cleaning up the inactive producers.  However, upon making a second request, the producer will be able to successfully establish a connection and proceed with the operation.

Make the initial request made on the new connection success.

(cherry picked from commit 711b621)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost cherry-picked/branch-2.10 cherry-picked/branch-2.11 cherry-picked/branch-3.0 cherry-picked/branch-3.1 doc-not-needed Your PR changes do not impact docs release/2.10.6 release/2.11.3 release/3.0.3 release/3.1.2 Stale type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants