-
Notifications
You must be signed in to change notification settings - Fork 9
Reconfiguration scheduling #11
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
Conversation
| // Clear running jobs | ||
| r.mu.Lock() | ||
| r.runningJobs = make(map[int64]*jobtypes.Job) | ||
| r.mu.Unlock() |
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.
go 中锁的写法一般在函数开头
使用 defer 释放资源,defer 总是在退出函数时执行
func (r *Runner) Close() error {
r.mu.Lock()
defer r.mu.Unlock()
r.runningJobs = make(map[int64]*jobtypes.Job)
}
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.
Ok, I will correct this problem. Thank you for your suggestion!
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.
我觉得这个文件不用删掉,项目确实得有一个地方入口以供测试功能(单测除外
|
|
||
| func startRunners(ctx context.Context, cfg *clrServer.Server) error { | ||
|
|
||
| // Initialize all collectors before starting runners |
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.
我觉得可以将这部分移动到 job 内部完成 这样项目整体结构会更好理解一些
从 server 启动依赖组件,collector task 也是 job 的一部分?你怎么看 @TJxiaobao
| "hertzbeat.apache.org/hertzbeat-collector-go/internal/util/logger" | ||
| ) | ||
|
|
||
| // init 函数在包被导入时自动执行 |
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.
可以换成英文注释
|
|
||
| // init 函数在包被导入时自动执行 | ||
| // 集中注册所有协议的工厂函数 | ||
| func init() { |
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.
这里可能仍然有一些出入,不过问题不大。这种方式也挺好的,显式配置更好
| "fmt" | ||
| "time" | ||
|
|
||
| _ "hertzbeat.apache.org/hertzbeat-collector-go/internal/collector/basic" |
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.
这里是预期中的吗?
它会执行 bacis 包下的所有 init 函数
| } | ||
|
|
||
| // NewResultHandler creates a new result handler | ||
| func NewResultHandler(logger logger.Logger) *ResultHandlerImpl { |
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.
如果这里是实现了一个接口的话,我想这里应该返回的是接口类型,而不是结构体的实现
| // 4. Triggering alerts based on thresholds | ||
| // 5. Updating monitoring status | ||
|
|
||
| if data.Code == 200 { |
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.
200 是魔法值,可以用 http 库的 200 常量来判断
| } | ||
|
|
||
| // NewCommonDispatcher creates a new common dispatcher | ||
| func NewCommonDispatcher(logger logger.Logger, metricsCollector MetricsCollector, resultHandler ResultHandler) *CommonDispatcherImpl { |
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.
这里应该创建一个 ctx 吗?
从 go 的编程风格来说,ctx 应该永远被用作函数的第一个参数实现,而不是放到结构体里面去,这不符合 go 的编程规范(约定俗成
| commonDispatcher MetricsTaskDispatcher | ||
| cyclicTasks sync.Map // map[int64]*jobtypes.Timeout for cyclic jobs | ||
| tempTasks sync.Map // map[int64]*jobtypes.Timeout for one-time jobs | ||
| ctx context.Context |
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.
同上,ctx 不应该作为结构体的属性
| ) | ||
|
|
||
| // hashedWheelTimer implements a time wheel for efficient timeout management | ||
| type hashedWheelTimer struct { |
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.
小写?
| bucket.mu.RUnlock() | ||
| } | ||
|
|
||
| return map[string]interface{}{ |
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.
可以使用 any 代替 interface {}
| if err := r.timeDispatch.AddJob(job); err != nil { | ||
| delete(r.runningJobs, job.ID) | ||
| r.Logger.Error(err, "failed to add job to time dispatcher", "jobID", job.ID) | ||
| return fmt.Errorf("failed to add job to time dispatcher: %w", err) |
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.
使用 errors.New() 以返回一个 error
| // Runner implements the service runner interface | ||
| type Runner struct { | ||
| Config | ||
| timeDispatch TimeDispatcher |
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.
这些配置应该都在 Config 中,config 是当前 server 的所有 ctx 配置,runner 只负责将当前 server run 起来
Based on the existing project framework, refactor the scheduling module of hertzbeat-collector-go.