Skip to content

Delete PartitionedConsumerImpl, use TopicsConsumerImpl instead #1365

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
Merged
merlimat merged 5 commits into from
Apr 4, 2018
Merged

Delete PartitionedConsumerImpl, use TopicsConsumerImpl instead #1365

merlimat merged 5 commits into from
Apr 4, 2018

Conversation

@hellostreaming
Copy link
Contributor

@hellostreaming hellostreaming commented Mar 10, 2018 •
edited
Loading

Motivation

Currently we have both TopicsConsumerImpl and PartitionedConsumerImpl. The behaviour and code of them are similar: contains a list of ConsumerImpl and make them work together. This PR tries to delete PartitionedConsumerImpl and use TopicsConsumerImpl instead.

Modifications

Delete PartitionedConsumerImpl, use TopicsConsumerImpl instead.
Fix errors in TopicsConsumerImpl.

Result

No api changes.

@hellostreaming hellostreaming changed the title elete partitionedConsumer, use topicsConsumer instead Delete PartitionedConsumerImpl, use TopicsConsumerImpl instead Mar 10, 2018
@hellostreaming
Copy link
Contributor Author

hellostreaming commented Mar 13, 2018

@merlimat @sijie Would you please help review this. Thanks.

sijie
sijie reviewed Mar 13, 2018
View reviewed changes
pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicsConsumerImpl.java Outdated
Copy link
Member

@sijie sijie Mar 13, 2018

Choose a reason for hiding this comment

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

any reason that topic is removed before closing the actual consumers?

Copy link
Contributor Author

@hellostreaming hellostreaming Mar 13, 2018

Choose a reason for hiding this comment

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

oh, Thanks, will move it into line 746, when all consumers closed.

@hellostreaming
Copy link
Contributor Author

hellostreaming commented Mar 15, 2018

rebased master, checked no leaking change in PartitionedConsumerImpl

@hellostreaming
Copy link
Contributor Author

hellostreaming commented Mar 18, 2018

retest this please

1 similar comment
@hellostreaming
Copy link
Contributor Author

hellostreaming commented Mar 18, 2018

retest this please

@hellostreaming
Copy link
Contributor Author

hellostreaming commented Mar 30, 2018

@merlimat Would you please help review this?

merlimat
merlimat reviewed Apr 1, 2018
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

I don't see the PartitionedConsumerImpl class being removed in this PR. Is that intentional?

...oker/src/test/java/org/apache/pulsar/client/impl/PerMessageUnAcknowledgedRedeliveryTest.java Outdated
Copy link
Contributor

@merlimat merlimat Mar 14, 2018

Choose a reason for hiding this comment

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

I think we could also rename TopicsConsumerImpl into MultiTopicsConsumerImpl, because TopicsConsumerImpl doesn't immediately convey that it's working on a collection of topics.

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java Outdated
Copy link
Contributor

@merlimat merlimat Apr 1, 2018

Choose a reason for hiding this comment

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

How does TopicsConsumerImpl discover the partitions it has to use?

Copy link
Contributor Author

@hellostreaming hellostreaming Apr 2, 2018

Choose a reason for hiding this comment

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

Thanks, this is similar to what we did in partitioned consumer, mainly by getPartitionedTopicMetadata.
In TopicsConsumerImpl we call subscribeAsync(topic) for the topic in conf, and this will call getPartitionedTopicMetadata again.

Copy link
Contributor

@merlimat merlimat Apr 2, 2018

Choose a reason for hiding this comment

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

Ok, though since we already know the number of partitions, we already know the list of partition names. Couldn't we just build the list and pass it on to the MultiTopicsConsumerImpl?

Copy link
Contributor Author

@hellostreaming hellostreaming Apr 2, 2018

Choose a reason for hiding this comment

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

Thanks, will do the change to avoid the dup calling of get metadata.

@hellostreaming
Copy link
Contributor Author

hellostreaming commented Apr 2, 2018

@merlimat, thanks for the review. PartitionedConsumerImpl should be remove, but was wrongly get back when do last rebase, now removed it.

merlimat
merlimat reviewed Apr 2, 2018
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

Change looks good. I just have one comment on the initialization path.

@merlimat merlimat added the type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use label Apr 2, 2018
@merlimat merlimat added this to the 2.0.0-incubating milestone Apr 2, 2018
@hellostreaming hellostreaming force-pushed the issue_1236 branch 3 times, most recently from 920a966 to ff2b969 Compare April 3, 2018 06:51
@hellostreaming
Copy link
Contributor Author

hellostreaming commented Apr 3, 2018 •
edited
Loading

seems CI build error could fix with #1489, may need retry after it merged

@hellostreaming
Copy link
Contributor Author

hellostreaming commented Apr 3, 2018

retest this please

@merlimat
Copy link
Contributor

merlimat commented Apr 3, 2018

@zhaijack please merge this branch with master to get the compilation fixed

@hellostreaming
Copy link
Contributor Author

hellostreaming commented Apr 4, 2018

Seems there was no conflict, lt should be well.

merlimat
merlimat approved these changes Apr 4, 2018
View reviewed changes
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@merlimat merlimat merged commit 1dd9c43 into apache:master Apr 4, 2018
@hellostreaming hellostreaming mentioned this pull request Apr 11, 2018
Closed
@shibd shibd mentioned this pull request Aug 7, 2022
Merged
1 task
RobertIndie pushed a commit that referenced this pull request Aug 31, 2022
@shibd
[refactor][client c++] Delete PartitionedConsumerImpl, use MultiTopic…
3a3ae23 
…sConsumerImpl instead (#16969)

### Motivation

The code of the C++ client is still relatively old, after #1365, the java client used `MultiTopicConsumerImpl` instead of `PartitionedConsumerImpl`. 

### Modifications
- Delete PartitionedConsumerImpl, use MultiTopicsConsumerImpl instead.
- For MultiTopicConsumerImpl, support seek message and topic partition listener feature.
BewareMyPower pushed a commit that referenced this pull request Sep 13, 2022
@shibd @BewareMyPower
[refactor][client c++] Delete PartitionedConsumerImpl, use MultiTopic…
23da64b 
…sConsumerImpl instead (#16969)

### Motivation

The code of the C++ client is still relatively old, after #1365, the java client used `MultiTopicConsumerImpl` instead of `PartitionedConsumerImpl`.

### Modifications
- Delete PartitionedConsumerImpl, use MultiTopicsConsumerImpl instead.
- For MultiTopicConsumerImpl, support seek message and topic partition listener feature.

(cherry picked from commit 3a3ae23)
BewareMyPower pushed a commit that referenced this pull request Sep 13, 2022
@shibd @BewareMyPower
[refactor][client c++] Delete PartitionedConsumerImpl, use MultiTopic…
eafd4c2 
…sConsumerImpl instead (#16969)

### Motivation

The code of the C++ client is still relatively old, after #1365, the java client used `MultiTopicConsumerImpl` instead of `PartitionedConsumerImpl`.

### Modifications
- Delete PartitionedConsumerImpl, use MultiTopicsConsumerImpl instead.
- For MultiTopicConsumerImpl, support seek message and topic partition listener feature.

(cherry picked from commit 3a3ae23)
BewareMyPower pushed a commit that referenced this pull request Sep 13, 2022
@shibd @BewareMyPower
[refactor][client c++] Delete PartitionedConsumerImpl, use MultiTopic…
b5ef559 
…sConsumerImpl instead (#16969)

### Motivation

The code of the C++ client is still relatively old, after #1365, the java client used `MultiTopicConsumerImpl` instead of `PartitionedConsumerImpl`.

### Modifications
- Delete PartitionedConsumerImpl, use MultiTopicsConsumerImpl instead.
- For MultiTopicConsumerImpl, support seek message and topic partition listener feature.

(cherry picked from commit 3a3ae23)
BewareMyPower pushed a commit that referenced this pull request Sep 13, 2022
@shibd @BewareMyPower
[refactor][client c++] Delete PartitionedConsumerImpl, use MultiTopic…
588a0a5 
…sConsumerImpl instead (#16969)

### Motivation

The code of the C++ client is still relatively old, after #1365, the java client used `MultiTopicConsumerImpl` instead of `PartitionedConsumerImpl`.

### Modifications
- Delete PartitionedConsumerImpl, use MultiTopicsConsumerImpl instead.
- For MultiTopicConsumerImpl, support seek message and topic partition listener feature.

(cherry picked from commit 3a3ae23)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 16, 2022
@shibd @nicoloboschi
[refactor][client c++] Delete PartitionedConsumerImpl, use MultiTopic…
91471e7 
…sConsumerImpl instead (apache#16969)

### Motivation

The code of the C++ client is still relatively old, after apache#1365, the java client used `MultiTopicConsumerImpl` instead of `PartitionedConsumerImpl`.

### Modifications
- Delete PartitionedConsumerImpl, use MultiTopicsConsumerImpl instead.
- For MultiTopicConsumerImpl, support seek message and topic partition listener feature.

(cherry picked from commit 3a3ae23)
(cherry picked from commit 23da64b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

Assignees

@hellostreaming hellostreaming

Labels

type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use

Projects

None yet

Milestone

2.0.0-incubating

Development

Successfully merging this pull request may close these issues.

4 participants

@hellostreaming @merlimat @sijie @jiazhai