-
Notifications
You must be signed in to change notification settings - Fork 766
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
feature:支持扩展,目前先支持DtpExecutor和ScheduledDtpExecutor的扩展 #288
Conversation
Nice pr, we will review it. |
import java.util.concurrent.ThreadFactory; | ||
import java.util.concurrent.ThreadPoolExecutor; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace with a single class import
import java.util.concurrent.ThreadFactory; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.TimeoutException; | ||
import java.util.concurrent.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace with a single class import
core/pom.xml
Outdated
<dependency> | ||
<groupId>cglib</groupId> | ||
<artifactId>cglib</artifactId> | ||
<version>3.3.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put the cglib version in dependencies/pom.xml
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Set; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace with a single class import and we can use IDEA checkstyle plugins to check it (the file in .github/checkstyle/checkstyle.xml)
@@ -0,0 +1,38 @@ | |||
package org.dromara.dynamictp.core.plugin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add Apache Licenses to the class header
forget to set IDEA automatic package guidance prohibits import*,resulting in package merging |
Modified, thank you for your CR |
|
||
public static Object resolveObject(String path) { | ||
if (path == null) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use StringUtils.isBlack()
core/src/main/java/org/dromara/dynamictp/core/plugin/DtpExtensionProxy.java
Show resolved
Hide resolved
enhancer.setSuperclass(target.getClass()); | ||
enhancer.setCallback(new DtpExtensionProxy(target, interceptor, signatureMap)); | ||
// 需要在DtpExecutor中添加一个默认无参构造函数 | ||
return enhancer.create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to provide a way created through a constructor with parameters.
} | ||
DtpExtension dtpExtension = (DtpExtension) ReflectionUtil.resolveObject(extension.getExtensionPath()); | ||
DtpRegistry.registerExtension(dtpExtension); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be extracted into a registration method.
@@ -61,8 +61,11 @@ public Object postProcessAfterInitialization(@NonNull Object bean, @NonNull Stri | |||
if (!(bean instanceof ThreadPoolExecutor) && !(bean instanceof ThreadPoolTaskExecutor)) { | |||
return bean; | |||
} | |||
// 在这里对所有我们需要代理的bean进行,当然这种方式适合代理加载到IOC的bean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For unity, it is better to use English comments.
@@ -138,6 +139,11 @@ public class DtpExecutor extends ThreadPoolExecutor | |||
*/ | |||
protected int awaitTerminationSeconds = 0; | |||
|
|||
public DtpExecutor() { | |||
this(10, 50, 100, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very inelegant.
*/ | ||
@EnableDynamicTp | ||
@SpringBootApplication | ||
public class ExtensionExampleApplication { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, this is an example, not a test case, and can be placed under any example.
@@ -186,6 +207,7 @@ public static void refresh(DtpProperties dtpProperties) { | |||
private static void refresh(ExecutorWrapper executorWrapper, DtpExecutorProps props) { | |||
if (props.coreParamIsInValid()) { | |||
log.error("DynamicTp refresh, invalid parameters exist, properties: {}", props); | |||
// TODO 对于error日志如何处理 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be changed to throw exception and catch exception.
|
||
default Object plugin(Object target) { | ||
return DtpExtensionProxyFactory.wrap(target, this); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an overloaded method.
default Object plugin(Object target, Class[] argumentTypes, Object[] arguments) { }
@@ -70,6 +73,8 @@ public class DtpRegistry implements ApplicationRunner { | |||
*/ | |||
private static final Map<String, ExecutorWrapper> EXECUTOR_REGISTRY = new ConcurrentHashMap<>(); | |||
|
|||
private static final List<DtpExtension> EXTENSIONS = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to maintain it separately in an ExtensionRegistry.
TimeUnit.MILLISECONDS, | ||
dtpExecutor.getQueue(), | ||
new NamedThreadFactory(dtpExecutor.getThreadPoolName()), | ||
RejectHandlerGetter.buildRejectedHandler(dtpExecutor.getRejectHandlerType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't recreate, just get the original.
* Max free memory for MemorySafeLBQ, unit M | ||
*/ | ||
private int maxFreeMemory = 16; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are not used, why add these two fields?
No description provided.