Skip to content
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

use outside ctx to make control #559

Closed
wants to merge 1 commit into from
Closed

use outside ctx to make control #559

wants to merge 1 commit into from

Conversation

gorilla001
Copy link

No description provided.

@codecov-io
Copy link

Codecov Report

Merging #559 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master     #559   +/-   ##
=======================================
  Coverage   19.18%   19.18%           
=======================================
  Files          28       28           
  Lines        1496     1496           
=======================================
  Hits          287      287           
  Misses       1170     1170           
  Partials       39       39

@cmingxu
Copy link
Contributor

cmingxu commented May 8, 2017

设计的过程是 leadershipLeader 和 leadershipFollower可以来回切换;
所以一次leader => follower是一个context;

@pwzgorilla
我理解你修改后的做法, 这样的做法是一个manager生命周期的的context, 程序启动context开始, 程序结束context结束; 这和预期的不太一样。

建议可以独立的context来做graceful机制

另外, 之所以以前没有把signal handler加上的原因是考虑到swan作为long running任务来运行, 手动shutdown的机会基本不存在;

另外如果想通过信号来做graceful的shutdown, 我理解重头戏是比如scheduler内部状态的graceful, events的graceful, 有很多工作需要做。


setupLogger(conf.LogLevel)
setupLogger(conf.LogLevel)
Copy link
Contributor

Choose a reason for hiding this comment

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

use tabs instead of white space

Copy link
Contributor

Choose a reason for hiding this comment

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

fmt一下就是。

@gorilla001
Copy link
Author

gorilla001 commented May 8, 2017

@cmingxu 我这样改是发现 src/cmd/manager_cmd:46 行的context.TODO() 没有做控制,从而导致src/manager/manager.go:180行的ctx.Done()无法执行到。而且148,152,156,160行任何一个goroutine出错推出,都没有显示的去退出其他goroutine,而只是在程序最后结束时调用了os.Exit(1)来清理(这样因该也是可行的)。所以才想到用一个全局的context来控制。当然也可以不用这样做,在172行调用stopFunc()也可以退出其他goroutine.

@cmingxu
Copy link
Contributor

cmingxu commented May 8, 2017

@pwzgorilla 我对从外面传入ctx没有意见;
我关注的地方在于改后没有体现每次从leader到follower的来回切换是一个context这个点;
另外: LeadershipFollower和LeadershipLeader不是对应的, 不知道你注意没有; LeadershipFollower event会出现多次

改动时候开多个manager试几次

@xiaods
Copy link
Contributor

xiaods commented May 8, 2017

要多做测试,有必要需要把每个函数的行为定义写在comments里面。目前的代码逻辑还是比较复杂的。需要简化逻辑。

go func() {
manager.criticalErrorChan <- eventbus.Start(stopCtx)
errC <- eventbus.Start(stopCtx)
Copy link
Contributor

Choose a reason for hiding this comment

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

这里我有一个小疑问 如果这四个里面都用一个 stopCtx 一旦外面 cancel 是否四个都能收到消息

Copy link
Contributor

Choose a reason for hiding this comment

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

需要做测试才能验证这个事情。

if err != nil {
return nil, err
}

err = store.InitZkStore(managerConf.ZkPath)
err = store.InitZkStore(cfg.ZkPath)
if err != nil {
logrus.Fatalln(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

return error?

go func() {
manager.criticalErrorChan <- eventbus.Start(stopCtx)
errC <- eventbus.Start(stopCtx)
Copy link
Contributor

Choose a reason for hiding this comment

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

errC太随意。errCh or errChan

Copy link
Contributor

@xiaods xiaods left a comment

Choose a reason for hiding this comment

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

这个Patch,一个是改conf到cfg,一个是调整逻辑。混合在一起让我很混淆。最好分开提交。

@xiaods
Copy link
Contributor

xiaods commented May 10, 2017

PR需要拆一下,把精华抽取出来force push一下。

@xiaods
Copy link
Contributor

xiaods commented May 10, 2017

need rebase

@gorilla001 gorilla001 closed this May 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants