[ISSUE #2856]Adjust the use of thread pools (naming module) #3136
Conversation
|
submit the part first and then modify the rest , please help to review this pr @chuntaojun @KomachiSion @zongtanghu |
| * @author gagharv | ||
| */ | ||
| @SuppressWarnings({"checkstyle:indentation", "PMD.ThreadPoolCreationRule"}) | ||
| public class NamingExecutor { |
There was a problem hiding this comment.
Would chain calls be better?
for example
NamingExecutor.mysqlCheckExecutor().xxxThere was a problem hiding this comment.
This will cause the thread pool to be exposed to the outside but the user can call shutdown by himself.
There was a problem hiding this comment.
maybe this idea should implemented wrapped for ThreadPoolExecutor
There was a problem hiding this comment.
I think this implementation is too complicated.
The GlobalExecutor is a logic wrapper for Executors, we only need to put the creating and managing down to ExecutorPoolManager.
Also, you can enhance the class name and method name of GlobalExecutor to make it more readlable.
Change-Id: I626179bd9ee8e852d9d51787950ad80744ec71cd
|
@KomachiSion how about this |
|
Plesase help to reivew this pr together, @chuntaojun @KomachiSion , thanks very much. |
|
|
||
|
|
||
| private AtomicInteger connectIdGenerator = new AtomicInteger(0); | ||
|
|
||
|
|
||
| private Map<Integer, StreamObserver<Resources>> connnections = new ConcurrentHashMap<>(16); | ||
|
|
||
|
|
||
| private Map<String, Resource> resourceMap = new ConcurrentHashMap<>(16); | ||
|
|
||
|
|
||
| private Map<String, String> checksumMap = new ConcurrentHashMap<>(16); | ||
|
|
||
|
|
||
| private static final String SERVICE_NAME_SPLITTER = "nacos"; | ||
|
|
||
|
|
||
| private static final String MESSAGE_TYPE_URL = "type.googleapis.com/istio.networking.v1alpha3.ServiceEntry"; | ||
|
|
||
|
|
||
| private static final long MCP_PUSH_PERIOD_MILLISECONDS = 10000L; | ||
|
|
||
|
|
||
| @Autowired | ||
| private ServiceManager serviceManager; | ||
|
|
||
|
|
||
| @Autowired | ||
| private IstioConfig istioConfig; | ||
|
|
There was a problem hiding this comment.
yes, code format
| private static final ScheduledExecutorService NAMING_TIMER_EXECUTOR = ExecutorFactory.Managed | ||
| .newScheduledExecutorService(ClassUtils.getCanonicalName(NamingApp.class), | ||
| Runtime.getRuntime().availableProcessors() * 2, | ||
| new NameThreadFactory("com.alibaba.nacos.naming.timer")); |
There was a problem hiding this comment.
Use public final static String variable to represent the const value "com.alibaba.nacos.naming.timer" is better? The same is as below.
There was a problem hiding this comment.
If set to public, there is no need to provide a method
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
#2856
Brief changelog
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=trueto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.