Skip to content

Revert #16603 "[enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread" #16803

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.

Sign up for GitHub

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

Jump to bottom
Closed
BewareMyPower wants to merge 1 commit into from
Closed

Revert #16603 "[enh] Broker - Shared subscription: run filters in a separate (per-subscription) thread" #16803

BewareMyPower wants to merge 1 commit into from

Conversation

@BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Jul 26, 2022 •
edited
Loading

Fixes #16802

Motivation

We should not run sendMessagesToConsumers in a separated thread. If the execute order is

readMoreEntries (1st) -> readEntriesComplete (1st) -> readMoreEntries (2nd) -> sendMessagesToConsumers (1st)

The replay queue would be read repeatedly.

Modifications

Revert #16603.

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • 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)

@BewareMyPower
Revert "[enh] Broker - Shared subscription: run filters in a separate…
637e5f6 
… (per-subscription) thread (dispatcherDispatchMessagesInSubscriptionThread) (apache#16603)"

Fixes apache#16802

This reverts commit abe5d15.
@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug release/blocker Indicate the PR or issue that should block the release until it gets resolved labels Jul 26, 2022
@BewareMyPower BewareMyPower added this to the 2.11.0 milestone Jul 26, 2022
@BewareMyPower BewareMyPower self-assigned this Jul 26, 2022
@eolivelli
Copy link
Contributor

eolivelli commented Jul 26, 2022

Instead of reverting, can we change the default to false?
I will look deeper into the problem

@BewareMyPower
Copy link
Contributor Author

BewareMyPower commented Jul 27, 2022

@eolivelli Just have a private discussion with @mattisonchao, he also suggested only changing the default value.

However, IMO, keeping this option could still bring some potential risks. We have to note that if you're enabling this option, duplicated messages might be received. At the time, there is something wrong with this implementation. I think we should revert the implementation and only add it again after fixing the bug.

@codelipenghui
Copy link
Contributor

codelipenghui commented Jul 27, 2022

@eolivelli I think we can change to false first and you can continue to check the details of this issue. If the issue cannot be resolved before the 2.11.0 release, we should revert this one. Because after users enable this option, they will get duplicated messages.

@BewareMyPower Could you please open a separate PR to change the default value to false? Just keep this one to track if we need revert or not.

@mattisonchao
Copy link
Member

mattisonchao commented Jul 27, 2022

@eolivelli Just have a private discussion with @mattisonchao, he also suggested only changing the default value.

However, IMO, keeping this option could still bring some potential risks. We have to note that if you're enabling this option, duplicated messages might be received. At the time, there is something wrong with this implementation. I think we should revert the implementation and only add it again after fixing the bug.

@BewareMyPower
Yes, I change my mind. After a deep look at this PR, I realise revert is better than changing it to false because this PR involved an issue and was originally just a performance issue.

Hi, @eolivelli
Could you provide a fix at the release 2.11 period? If not, we can revert this PR before release 2.11.

@BewareMyPower
Copy link
Contributor Author

BewareMyPower commented Jul 27, 2022

I agree. Let's keep this PR open and I'll push another PR to disable the option.

BewareMyPower added a commit to BewareMyPower/pulsar that referenced this pull request Jul 27, 2022
@BewareMyPower
[fix][broker] Run sendMessagesToConsumers in the same thread by default
d05754e 
### Motivation

See apache#16802 and the discussion in
apache#16803. Before reverting apache#16603,
disabling the `dispatcherDispatchMessagesInSubscriptionThread` option
first.

### Modifications

Change the default value of
`dispatcherDispatchMessagesInSubscriptionThread` to false.
@BewareMyPower BewareMyPower mentioned this pull request Jul 27, 2022
Closed
4 tasks
@eolivelli
Copy link
Contributor

eolivelli commented Jul 27, 2022

@mattisonchao @BewareMyPower @codelipenghui
I am investigating on the problem today

@BewareMyPower
Copy link
Contributor Author

BewareMyPower commented Jul 27, 2022

Close it due to #16812

@BewareMyPower BewareMyPower deleted the bewaremypower/revert-16603 branch July 27, 2022 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari Awaiting requested review from lhotari

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@dlg99 dlg99 Awaiting requested review from dlg99

@eolivelli eolivelli Awaiting requested review from eolivelli

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

Assignees

@BewareMyPower BewareMyPower

Labels

release/blocker Indicate the PR or issue that should block the release until it gets resolved type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

Repeated messages of shared dispatcher

4 participants

@BewareMyPower @eolivelli @codelipenghui @mattisonchao