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

[ISSUE #218] Fix spring scopeTarget will repeat consumer instance #210

Merged
merged 23 commits into from
Feb 10, 2020

Conversation

einsitang
Copy link
Contributor

@einsitang einsitang commented Dec 20, 2019

What is the purpose of the change

change consumer listener use SpringContext getBeansWithAnnotation will get same bean with scopeTarget ,cause repeat consumer client with same clientId(IP@PID on rocketmq-client 4.6.0)

Brief changelog

fix scopeTarget bean instance same clientId

Verifying this change

use @RefreshScope on RocketMQListener , will registration is the same clientId on RocketMQ-Console consumer client (same IP@PID),will thorws error (log : has been created before, specify another name please.) , but only on rocketmq-client version >= 4.6.0

at forfuns:develop-pr branch will fixed

Copy link
Contributor

@RongtongJin RongtongJin left a comment

Choose a reason for hiding this comment

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

I think it's a good enhancement, but the implementation is a bit complex.
Firstly, it is unnecessary to expose client instance name to users in RocketMQProperties explicitly. Users can set consumer instance name in prepareStart method If he want to do that.
Secondly, why not just filter the proxy bean in afterSingletonsInstantiated method? Writing a SpringBeanUtil class make the code complex.
Finally, It would be better if you can write some unit tests.

demo.rocketmq.myNameServer=dev1.host.70yi.ren:9876
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change back to 127.0.0.1

@RongtongJin RongtongJin added the enhancement New feature or request label Dec 24, 2019
@RongtongJin
Copy link
Contributor

Could you open a issue first?

@einsitang
Copy link
Contributor Author

@RongtongJin sorry busy for work , open issue late :-)

@einsitang
Copy link
Contributor Author

issue : #218

@einsitang
Copy link
Contributor Author

resolve conflict with latest master

@RongtongJin RongtongJin changed the title fixed spring scopeTarget will repeat consumer instance [ISSUE #218] Fixed spring scopeTarget will repeat consumer instance Dec 30, 2019
@RongtongJin RongtongJin changed the title [ISSUE #218] Fixed spring scopeTarget will repeat consumer instance [ISSUE #218] Fix spring scopeTarget will repeat consumer instance Dec 30, 2019
@RongtongJin RongtongJin added this to the 2.1.0 milestone Jan 10, 2020
@vongosling
Copy link
Member

vongosling commented Jan 15, 2020

@snicoll Could you help to review this pr:-) refresh could result in the two instances of the **Container created. But I feel spring has provided a similar elegant method like "SpringBeanUtil" here, we do not want to create an extra class to deal with this problem.

@snicoll
Copy link
Contributor

snicoll commented Jan 15, 2020

Can do when time permits. I'd appreciate an answer here: #171 (comment)

@vongosling
Copy link
Member

@snicoll Thank you in advance, we hope this feature can be released in version 2.1.0 in the latest week, I really appreciate your time:-)

Copy link
Contributor

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Calling ScopedProxyUtils.isScopedTarget(beanName) is what you should be doing there. There isn't an infrastructure for such use cases as each detection logic is slightly different so it's hard to create a common API that would suits all needs.

* @param clazz annotation class
* @return beans map without proxyTarget bean
*/
public static Map<String, Object> getBeansWithAnnotation(@NonNull ConfigurableApplicationContext applicationContext, Class<? extends Annotation> clazz) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather move the check in the code that's calling it. This also prevents the creation of a util class

@RongtongJin RongtongJin merged commit 2853384 into apache:master Feb 10, 2020
@RongtongJin
Copy link
Contributor

I will merge the PR first, then modify the code according to @snicoll .

1 similar comment
@RongtongJin
Copy link
Contributor

I will merge the PR first, then modify the code according to @snicoll .

RongtongJin added a commit to RongtongJin/rocketmq-spring that referenced this pull request Feb 10, 2020
RongtongJin added a commit that referenced this pull request Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants