Skip to content

Revert #3312 "Prevent dup consumers on same client cnx with shared subscrip#3743

Closed
jiazhai wants to merge 2 commits intoapache:masterfrom
jiazhai:fix-sub
Closed

Revert #3312 "Prevent dup consumers on same client cnx with shared subscrip#3743
jiazhai wants to merge 2 commits intoapache:masterfrom
jiazhai:fix-sub

Conversation

@jiazhai
Copy link
Member

@jiazhai jiazhai commented Mar 4, 2019

This reverts commit 231db03.
which is commit in #3312

@jiazhai jiazhai added this to the 2.3.1 milestone Mar 4, 2019
@jiazhai jiazhai requested review from merlimat and sijie March 4, 2019 08:28
@jiazhai
Copy link
Member Author

jiazhai commented Mar 4, 2019

@lovelle This fix has some issue. a subscribe is identified by "sub-name" + "topic-name".
Would like to revert the change in #3312 .

@sijie
Copy link
Member

sijie commented Mar 4, 2019

@jiazhai can you describe more on why you revert this commit? this would give context for other reviewers to review.

@lovelle please also review and discuss with @jiazhai why reverting this.

@jiazhai
Copy link
Member Author

jiazhai commented Mar 4, 2019

@sijie @lovelle Here is the error that user reported:
wechatimg427

subscribes with different topic-name and same subscribe-name should get different subscribe, but it returns same subscribe instance.

@lovelle
Copy link
Contributor

lovelle commented Mar 4, 2019

@jiazhai @sijie Oh sorry! I apologize for this inconvenience.

@jiazhai is absolutelly right about this issue, I already have a bugfix with tests exploiting what is described here.
ASAP I will post a pr with the fix and tests if you agree.

Sorry again.

@lovelle
Copy link
Contributor

lovelle commented Mar 4, 2019

@sijie @jiazhai #3746 should fix this issue, could you please take look?

Thanks!

@jiazhai
Copy link
Member Author

jiazhai commented Mar 5, 2019

@lovelle Thanks for the fix in #3746.
If it is reasonable for original use case(1 client has several share consumer with same subscribe), we may need to how to fix the original issue.
And also seems current way of fix leaks the check for pattern subscribe.

@jiazhai
Copy link
Member Author

jiazhai commented Mar 5, 2019

rerun java8 tests

lovelle added a commit to lovelle/pulsar that referenced this pull request Mar 6, 2019
Fixes apache#3743 issue.

Return previous instance of a consumer in the subscription processed should only
be considered with the scope of the same topic.

Modifications:

  - Remove optimization of duplicated consumers for multi topics subscribe and
    pattern topics subscribe, this should be handled with a different approach.
  - Filter consumers for the same topic name.
  - Filter consumers which are connected to broker, this is not necessary to fix
    this issue but is a good thing to do.
  - Add test that verifies that same subscription will allow different consumers
    instance for different topics.
@jiazhai jiazhai closed this Mar 7, 2019
@jiazhai
Copy link
Member Author

jiazhai commented Mar 7, 2019

close this issue for more discussion please reference pr #3746

@merlimat merlimat removed this from the 2.3.1 milestone Mar 7, 2019
merlimat pushed a commit that referenced this pull request Mar 7, 2019
…3746)

Fixes #3743 issue.

Return previous instance of a consumer in the subscription processed should only
be considered with the scope of the same topic.

Modifications:

  - Remove optimization of duplicated consumers for multi topics subscribe and
    pattern topics subscribe, this should be handled with a different approach.
  - Filter consumers for the same topic name.
  - Filter consumers which are connected to broker, this is not necessary to fix
    this issue but is a good thing to do.
  - Add test that verifies that same subscription will allow different consumers
    instance for different topics.
merlimat pushed a commit that referenced this pull request Mar 29, 2019
…3746)

Fixes #3743 issue.

Return previous instance of a consumer in the subscription processed should only
be considered with the scope of the same topic.

Modifications:

  - Remove optimization of duplicated consumers for multi topics subscribe and
    pattern topics subscribe, this should be handled with a different approach.
  - Filter consumers for the same topic name.
  - Filter consumers which are connected to broker, this is not necessary to fix
    this issue but is a good thing to do.
  - Add test that verifies that same subscription will allow different consumers
    instance for different topics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments