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

feat(sentinel-transport):make command center thread daemon #2181

Merged
merged 2 commits into from
Jun 4, 2021

Conversation

wutingjia
Copy link
Contributor

Describe what this PR does / why we need it

make sentinel client close resources more easier and elegant

Does this pull request fix one issue?

#2178

Describe how you did it

make commad center thread daemon

Describe how to verify it

Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Apr 30, 2021

CLA assistant check
All committers have signed the CLA.

@sczyh30 sczyh30 added the kind/enhancement Category issues or prs related to enhancement. label Apr 30, 2021
@sczyh30 sczyh30 requested a review from cdfive April 30, 2021 05:42
@Anilople
Copy link
Contributor

There are many place use com.alibaba.csp.sentinel.concurrent.NamedThreadFactory.NamedThreadFactory(String), how about change them all to daemon in this PR?

For example:

new ArrayBlockingQueue<Runnable>(1), new NamedThreadFactory("sentinel-nacos-ds-update"),

new ArrayBlockingQueue<Runnable>(1), new NamedThreadFactory("sentinel-zookeeper-ds-update"),

Copy link
Collaborator

@linlinisme linlinisme left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -48,7 +48,7 @@

@SuppressWarnings("PMD.ThreadPoolCreationRule")
private static final ExecutorService pool = Executors.newFixedThreadPool(10,
new NamedThreadFactory("dubbo-consumer-pool"));
new NamedThreadFactory("dubbo-consumer-pool", true));
Copy link
Member

Choose a reason for hiding this comment

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

For demo maybe it's not needed (waiting may be needed)?

@sczyh30 sczyh30 merged commit cfedea8 into alibaba:master Jun 4, 2021
@sczyh30
Copy link
Member

sczyh30 commented Jun 4, 2021

Thanks for contributing.

@sczyh30 sczyh30 added this to the 1.8.2 milestone Jun 4, 2021
sczyh30 pushed a commit that referenced this pull request Jun 4, 2021
* feat:make command center thread daemon
* feat:make all NamedThreadFactory thread daemon
linkolen pushed a commit to shivagowda/Sentinel that referenced this pull request Aug 14, 2021
* feat:make command center thread daemon
* feat:make all NamedThreadFactory thread daemon
linkolen pushed a commit to shivagowda/Sentinel that referenced this pull request Aug 16, 2021
* feat:make command center thread daemon
* feat:make all NamedThreadFactory thread daemon
Zhang-0952 pushed a commit to Zhang-0952/Sentinel that referenced this pull request Mar 4, 2022
* feat:make command center thread daemon
* feat:make all NamedThreadFactory thread daemon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] make sentinel client close resources more easier and elegant
6 participants