-
Notifications
You must be signed in to change notification settings - Fork 201
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
perf: 主机与主机关系事件处理优化 #1145 #1146
perf: 主机与主机关系事件处理优化 #1145 #1146
Conversation
jsonwan
commented
Jul 26, 2022
- 主机缓存更新策略优化;
- 主机、主机关系事件监听代码重构。
主机缓存更新策略优化; 主机事件监听代码重构。
主机关系事件监听代码重构。
代码重构:去除多个eventsHandler逻辑。
增加主机关系事件处理流水日志。
log.info("start to handle host event:{}", JsonUtils.toJson(event)); | ||
handleOneEventIndeed(event); | ||
} catch (Throwable t) { | ||
log.error(String.format("Fail to handle hostEvent:%s", event), t); |
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.
- String.format有性能问题,不推荐在日志中使用
- 日志格式化尽量保证全局统一,少用%s这种方式
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.
重构代码时老代码中直接拿过来的,已修改
} catch (Throwable t) { | ||
log.error(String.format("Fail to handle hostEvent:%s", event), t); | ||
} finally { | ||
log.info("end to handle host event"); |
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.
这句日志是否有必要?如果真的有必要,那么需要输出对应的事件,不然根本无法区分对应的是哪个
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.
已去除
hostInfoDTO.setIp(ip); | ||
if (!ip.contains(":")) { | ||
String cloudIp = cloudAreaId + ":" + ip; | ||
hostInfoDTO.setGseAgentAlive(queryAgentStatusClient.getAgentStatus(cloudIp).status == 1); |
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.
魔鬼数字,我记得应该有常量调试agent状态
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.
已修改
String cloudIp = cloudAreaId + ":" + ip; | ||
hostInfoDTO.setGseAgentAlive(queryAgentStatusClient.getAgentStatus(cloudIp).status == 1); | ||
} else { | ||
hostInfoDTO.setGseAgentAlive(queryAgentStatusClient.getAgentStatus(ip).status == 1); |
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.
魔鬼数字
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.
已修改
log.debug("handle hostRelationEvent:" + watch.prettyPrint()); | ||
} | ||
} catch (Throwable t) { | ||
log.error(String.format("Fail to handle hostRelationEvent of appId %d, event:%s", appId, event), t); |
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.
不建议使用String.format
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.
已修改
} catch (Throwable t) { | ||
log.error(String.format("Fail to handle hostRelationEvent of appId %d, event:%s", appId, event), t); | ||
} finally { | ||
log.info("end to handle host relation event"); |
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.
没有具体的事件信息
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.
已去除
updateHostCacheWhenRelCreated(hostTopoDTO); | ||
break; | ||
case ResourceWatchReq.EVENT_TYPE_UPDATE: | ||
log.warn("Unexpected event:hostRelation Update"); |
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.
如果不需要处理的话,能否直接忽略?而不是打印一个warn日志,反而容易引起误解(比如外部合作方会经常认为warn日志跟他们的问题有关)。我看default直接break就可以了
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.
已去除
"cannot find hostInfo by hostId:{}, wait for host event or sync", | ||
hostTopoDTO.getHostId() | ||
); | ||
log.warn(msg.getMessage()); |
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.
直接 log.warn("cannot find hostInfo by hostId:{}, wait for host event or sync", hostTopoDTO.getHostId()) 就可以了
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.
已修改
); | ||
hostRelationWatchRedisKeyHeartBeatThread.setName("hostRelationWatchRedisKeyHeartBeatThread"); | ||
hostRelationWatchRedisKeyHeartBeatThread.start(); | ||
RedisKeyHeartBeatThread relationWatchBeatThread = startRedisKeyHeartBeatThread(); | ||
log.info("start watch hostRelation at {},{}", TimeUtil.getCurrentTimeStr("HH:mm:ss"), |
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.
TimeUtil.getCurrentTimeStr,已经包含了当前时间的意思了,为啥还要传入一个System.currentTimeInMills()??
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.
另外,DateUtil已经包含了该方法了
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.
看下时间的操作类能否整合下,比如改成DateTimeUtil
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.
TimeUtil.getCurrentTimeStr,已经包含了当前时间的意思了,为啥还要传入一个System.currentTimeInMills()??
已简化
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.
看下时间的操作类能否整合下,比如改成DateTimeUtil
两者使用场景不同,虽然有一些功能交集,但是整合在一起会更加臃肿,不建议整合
处理Review意见