Skip to content

Conversation

@EachannChan
Copy link
Contributor

@EachannChan EachannChan commented Dec 25, 2024

[#513] Virtual thread pin event metric and alarm

@yanhom1314
Copy link
Collaborator

代码改的太乱,改动太大了。我理解这是一个新功能,期望的应该是以最小代价、最小改动集成进��,对扩展开放,对修改关闭,而不是大量改动现有逻辑,分散在其中。

public static final String LARK_ALARM_JSON_COMMON_STR =
"{\"tag\":\"div\",\"fields\":[{\"is_short\":true,\"text\":{\"tag\":\"lark_md\",\"content\":\"%s\"}}]},";

public static final String LARK_ALARM_JSON_STR_SUFFIX =
Copy link
Collaborator

Choose a reason for hiding this comment

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

这是一个完整的json,不要拆分开,很难维护,格式容易配置错

/**
* Pin timeout alarm.
*/
PIN_TIMEOUT("pin_timeout", "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个不要归类到该枚举里,枚举里是线程池层面常规项,这个加入不太合适

/**
* 拓展字段
*/
private final Map<String, Double> extMap = new ConcurrentHashMap<>(2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

扩展字段类型可以是任意,不要用double,用Object,初始长度不用指定


List<NotifyItem> notifyItems = new ArrayList<>(6);
NotifyItem pinTimeoutNotify = new NotifyItem();
pinTimeoutNotify.setType(NotifyItemEnum.PIN_TIMEOUT.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个不要归类到现有类型里

private static void doRefresh(ExecutorWrapper executorWrapper, DtpExecutorProps props) {
ExecutorAdapter<?> executor = executorWrapper.getExecutor();
if (executorWrapper.isVirtualThreadExecutor()) {
doRefreshCommon(executorWrapper, props);
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个判断可以去掉?走现有逻辑

NotifyItem notifyItem = DtpNotifyCtxHolder.get().getNotifyItem();
BaseNotifyCtx notifyCtx = DtpNotifyCtxHolder.get();
if (notifyCtx.isToLog()) {
log.warn("DynamicTp alarm, executor [" + notifyCtx.getExecutorWrapper().getThreadPoolName() + "]: \n" + Arrays.toString(notifyCtx.getContent()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是业务无关逻辑,不要硬加判断搞的很乱

if (notifyCtx.isCommonNotify()) {
notifier.sendCommonAlarmMsg(p, notifyItemEnum, notifyCtx.getContent());
} else {
if (!notifyCtx.getExecutorWrapper().isVirtualThreadExecutor()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

同上


private boolean isCommonNotify;

private boolean isToLog;
Copy link
Collaborator

@yanhom1314 yanhom1314 Feb 12, 2025

Choose a reason for hiding this comment

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

不懂加这些字段意义是啥

}

@Override
public void sendCommonAlarmMsg(NotifyPlatform notifyPlatform, NotifyItemEnum notifyItemEnum, String... content) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

通用告警接口期望的是调用方传进来最终content,直接发送,不用再做组装

@EachannChan
Copy link
Contributor Author

编译时需要将core,spring模块的JDK版本更改为21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants