Skip to content

Conversation

liubao68
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 81.867% when pulling f848ffc on liubao68:master into e5bba88 on ServiceComb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 81.867% when pulling 9b6a8ad on liubao68:master into e5bba88 on ServiceComb:master.

public class LoadbalanceHandler extends AbstractHandler {
private static final Logger LOGGER = LoggerFactory.getLogger(LoadbalanceHandler.class);

private static final ExecutorService RETRY_POOL = Executors.newCachedThreadPool(new ThreadFactory() {
Copy link
Member

Choose a reason for hiding this comment

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

It could be better to use fix size thread pool to avoid the resource leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Is not a resource leak stuff, but a performance stuff I think. And I.think it's better to use a cached pool to reuse in high volume request and when no requests thread will be destroyed automatically

@Override
public void execute(Runnable command) {
command.run(); // retry的场景,对于同步调用, 需要在网络线程中进行。同步调用的主线程已经被挂起,无法再主线程中进行重试。
// retry的场景,对于同步调用, 同步调用的主线程已经被挂起,无法再主线程中进行重试;
Copy link
Member

Choose a reason for hiding this comment

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

can you reproduce this issue in a test and ensure it's fixed in the new solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's quite easy to reproduce according to the issue discretion, and I have test it.

Copy link
Contributor Author

@liubao68 liubao68 Jun 23, 2017

Choose a reason for hiding this comment

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

start a producer to listen to both rpc/highway, and a rest api that throws an exception. start a consumer with load balance retry enabled, and call this method

Copy link
Member

Choose a reason for hiding this comment

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

great, could you please add an automated test to ensure it won't be broken by changes in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 81.924% when pulling 124253b on liubao68:master into e5bba88 on ServiceComb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 81.924% when pulling 124253b on liubao68:master into e5bba88 on ServiceComb:master.

@WillemJiang WillemJiang merged commit 50740ca into apache:master Jun 27, 2017
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