Skip to content

[fix][broker] WaitForExclusive should fenced when exist Shared producer. - #18380

Closed
shibd wants to merge 1 commit into
apache:masterfrom
shibd:produce_access
Closed

[fix][broker] WaitForExclusive should fenced when exist Shared producer.#18380
shibd wants to merge 1 commit into
apache:masterfrom
shibd:produce_access

Conversation

@shibd

@shibd shibd commented Nov 8, 2022

Copy link
Copy Markdown
Member

Motivation

In the current implementation, if a Shared Producer is created first, and can be again created successfully a producer of the WaitForExclusive.

It's confuse. Because even if the Shared producer is disconnected, it will not trigger the WaitForExclusive producer to become exclusive.

Modifications

  • WaitForExclusive producer should fence when existing Shared producer.

Documentation

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

Matching PR in forked repository

PR in forked repository: shibd#20

@shibd
shibd marked this pull request as draft November 8, 2022 02:19
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Nov 8, 2022
@shibd
shibd marked this pull request as ready for review November 8, 2022 02:32
@liangyepianzhou liangyepianzhou changed the title [fix] WaitForExclusive should fenced when exist Shared producer. [fix][broker] WaitForExclusive should fenced when exist Shared producer. Nov 8, 2022
@Technoboy- Technoboy- added type/bug The PR fixed a bug or issue reported a bug area/broker ready-to-test labels Nov 8, 2022
@Technoboy- Technoboy- added this to the 2.12.0 milestone Nov 8, 2022
@Technoboy- Technoboy- closed this Nov 8, 2022
@Technoboy- Technoboy- reopened this Nov 8, 2022
@codecov-commenter

codecov-commenter commented Nov 8, 2022

Copy link
Copy Markdown

Codecov Report

Merging #18380 (674e7a8) into master (2e878e8) will increase coverage by 5.43%.
The diff coverage is 44.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18380      +/-   ##
============================================
+ Coverage     40.04%   45.48%   +5.43%     
- Complexity     8625    10809    +2184     
============================================
  Files           687      757      +70     
  Lines         67436    74036    +6600     
  Branches       7221     8116     +895     
============================================
+ Hits          27007    33675    +6668     
+ Misses        37411    36594     -817     
- Partials       3018     3767     +749     
Flag Coverage Δ
unittests 45.48% <44.00%> (+5.43%) ⬆️

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

Impacted Files Coverage Δ
.../org/apache/pulsar/broker/admin/AdminResource.java 66.58% <0.00%> (+<0.01%) ⬆️
...apache/pulsar/broker/admin/impl/FunctionsBase.java 0.00% <0.00%> (ø)
...org/apache/pulsar/broker/admin/impl/SinksBase.java 0.00% <0.00%> (ø)
...g/apache/pulsar/broker/admin/impl/SourcesBase.java 0.00% <0.00%> (ø)
...a/org/apache/pulsar/broker/admin/v2/Functions.java 0.00% <0.00%> (ø)
...java/org/apache/pulsar/broker/admin/v2/Worker.java 0.00% <0.00%> (ø)
...org/apache/pulsar/broker/admin/v2/WorkerStats.java 0.00% <0.00%> (ø)
...broker/delayed/InMemoryDelayedDeliveryTracker.java 68.33% <0.00%> (+3.33%) ⬆️
...rg/apache/pulsar/broker/service/AbstractTopic.java 64.67% <0.00%> (-0.05%) ⬇️
...ervice/persistent/MessageRedeliveryController.java 60.97% <ø> (ø)
... and 295 more

@poorbarcode poorbarcode left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, I don't agree. The user should have a scenario like this:

Users used multiple producers before and need to change to Exclusive now. In this case, if users want to smooth over, users need the coexistence of the producers of both modes Shared(old) and Exclusive(new)

@shibd shibd closed this Nov 9, 2022
@shibd shibd reopened this Nov 14, 2022
@shibd

shibd commented Nov 14, 2022

Copy link
Copy Markdown
Member Author

No, I don't agree. The user should have a scenario like this:

Users used multiple producers before and need to change to Exclusive now. In this case, if users want to smooth over, users need the coexistence of the producers of both modes Shared(old) and Exclusive(new)

I think we should keep the Exclusive functional single and should not allow both Shared and WaitForExclusive to appear at the same time. @merlimat Can you take a look at it?

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that a Exclusive producer should fence out all of the existing shared producers.

The point of WaitForExclusive is the you want to take control over the topic, but wait if there is another Exclusive producer connected or enquired as WaitForExclusive.

@shibd

shibd commented Nov 15, 2022

Copy link
Copy Markdown
Member Author

I think that a Exclusive producer should fence out all of the existing shared producers.

The point of WaitForExclusive is the you want to take control over the topic, but wait if there is another Exclusive producer connected or enquired as WaitForExclusive.

Isn't direct fencing a bit forcible? If the user wants to fence all existing Shared producers, he can use ExclusiveWithFencing mode.

@shibd
shibd requested review from eolivelli and removed request for Technoboy-, codelipenghui, congbobo184, eolivelli and gaoran10 November 30, 2022 02:11
@shibd
shibd requested review from eolivelli and poorbarcode and removed request for poorbarcode November 30, 2022 02:11
@shibd
shibd requested review from poorbarcode and removed request for eolivelli December 26, 2022 02:58
@shibd
shibd requested a review from eolivelli December 27, 2022 09:09

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure that we agree on this change.

A WaitForExclusive producer cannot complete by throwing 'Fenced'.
You are Fenced after becoming the owner, but in this case the Producer was never been elected as owner of the topic.

I think that we should behave like Exclusive and ExclusiveWithFencing: if there are Shared producers they must be disconnected and fail

@shibd

shibd commented Dec 28, 2022

Copy link
Copy Markdown
Member Author

@eolivelli Thanks for your reply.

I think that we should behave like Exclusive and ExclusiveWithFencing: if there are Shared producers they must be disconnected and fail

In fact, Exclusive's current behavior is that when it encounters has shared producer, it will fence itself.

} else if (!producers.isEmpty()) {
return FutureUtil.failedFuture(new ProducerFencedException("Topic has existing shared producers"));
} else if (producer.getTopicEpoch().isPresent()

In my opinion, this scenario occurs when the user connects to the wrong topic. We should tell Exclusive/WaitForExclusive producers about the exception of existing shared producers, rather than fence existing shared producers.

@congbobo184

Copy link
Copy Markdown
Contributor

my point is if the share producer connects first, the WaitForExclusive producer behavior should be like Exclusive. WaitForExclusive is only used if an Exclusive consumer has connected to the topic, if don't have an Exclusive consumer, its behavior doesn't have any difference from the Exclusive consumer.

@github-actions

Copy link
Copy Markdown

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

@github-actions github-actions Bot added the Stale label Jan 28, 2023
@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@shibd shibd closed this Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker doc-not-needed Your PR changes do not impact docs ready-to-test 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.

6 participants