main: signal related func better with context

[00arthur00]

change signature of func interrupt and reload with context.

[kakkoyun]

Thanks for your contribution.
However, I don't see any tangible reasons for this change. It doesn't improve anything. And there are controversial opinions on using context for just sake of cancellation.
https://dave.cheney.net/2017/08/20/context-isnt-for-cancellation

[00arthur00]

However, I don't see any tangible reasons for this change. It doesn't improve anything. And there are controversial opinions on using context for just sake of cancellation.

@kakkoyun

Under the cmd/thanos, other go files use context.WithCancel to perform the cancel semantic(the main file as well). The purpose of the PR is to keep same style of such semantic.

If you doesn't agree, I'm glad to close the PR.

[kakkoyun]

Hey @00arthur00, I have checked the usages of context patterns under thanos/cmd, I could only find one which is used solely for cancellation.

thanos/cmd/thanos/rule.go

Lines 493 to 519 in 601d4cf

	ctx, cancel := context.WithCancel(context.Background())
	g.Add(func() error {
	// Initialize rules.
	if err := reloadRules(logger, ruleFiles, ruleMgr, evalInterval, metrics); err != nil {
	level.Error(logger).Log("msg", "initialize rules failed", "err", err)
	return err
	}
	for {
	select {
	case <-reloadSignal:
	if err := reloadRules(logger, ruleFiles, ruleMgr, evalInterval, metrics); err != nil {
	level.Error(logger).Log("msg", "reload rules by sighup failed", "err", err)
	}
	case reloadMsg := <-reloadWebhandler:
	err := reloadRules(logger, ruleFiles, ruleMgr, evalInterval, metrics)
	if err != nil {
	level.Error(logger).Log("msg", "reload rules by webhandler failed", "err", err)
	}
	reloadMsg <- err
	case <-ctx.Done():
	return ctx.Err()
	}
	}
	}, func(error) {
	cancel()
	})
	}

All the others are passed along to the other APIs that requires context.Context.

Maybe we should fix that exceptional case, to be consistent.
So I'm closing this, for now, thank you for your contribution anyway!
Maybe you would want to fix that case 🤗

[00arthur00]

Hey @00arthur00, I have checked the usages of context patterns under thanos/cmd, I could only find one which is used solely for cancellation.

thanos/cmd/thanos/rule.go

Lines 493 to 519 in 601d4cf

	ctx, cancel := context.WithCancel(context.Background())
	g.Add(func() error {
	// Initialize rules.
	if err := reloadRules(logger, ruleFiles, ruleMgr, evalInterval, metrics); err != nil {
	level.Error(logger).Log("msg", "initialize rules failed", "err", err)
	return err
	}
	for {
	select {
	case <-reloadSignal:
	if err := reloadRules(logger, ruleFiles, ruleMgr, evalInterval, metrics); err != nil {
	level.Error(logger).Log("msg", "reload rules by sighup failed", "err", err)
	}
	case reloadMsg := <-reloadWebhandler:
	err := reloadRules(logger, ruleFiles, ruleMgr, evalInterval, metrics)
	if err != nil {
	level.Error(logger).Log("msg", "reload rules by webhandler failed", "err", err)
	}
	reloadMsg <- err
	case <-ctx.Done():
	return ctx.Err()
	}
	}
	}, func(error) {
	cancel()
	})
	}

All the others are passed along to the other APIs that requires context.Context.
Maybe we should fix that exceptional case, to be consistent.
So I'm closing this, for now, thank you for your contribution anyway!
Maybe you would want to fix that case

@kakkoyun Please see the comment.
The orignal one is close(ch), but bwplotka comment to use context.WithCancel.

Please see:
#1848 (comment)

[bwplotka]

Hah, that's true I proposed it. I think I can agree context is nice for consistency here. But really not a big deal I don't have a strong opinion. Any other thoughts? (:

…

On Mon, 30 Mar 2020 at 09:29, Arthur ***@***.***> wrote: Hey @00arthur00 <https://github.com/00arthur00>, I have checked the usages of context patterns under thanos/cmd, I could only find one which is used solely for cancellation. https://github.com/thanos-io/thanos/blob/601d4cf57a1e940208714b2c309f990834df958d/cmd/thanos/rule.go#L493-L519 All the others are passed along to the other APIs that requires context.Context. Maybe we should fix that exceptional case, to be consistent. So I'm closing this, for now, thank you for your contribution anyway! Maybe you would want to fix that case @kakkoyun <https://github.com/kakkoyun> Please see the comment. The orignal one is close(ch), but bwplotka comment to use context.WithCancel. Please see: #1848 (comment) <#1848 (comment)> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2306 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O5EH3RN4IW5P2ISZADRKBJ5VANCNFSM4LSKVIFQ> .

[kakkoyun]

I also don't have strong opinions on it as well. I find channels simpler.
I'm happy to reopen the PR if you all think context is way to go for the project.

[00arthur00]

I also don't have strong opinions on it as well. I find channels simpler.
I'm happy to reopen the PR if you all think context is way to go for the project.

@kakkoyun
I agree with bwplotka. Though not a big deal. Please help to reopen. I think we'd better keep consistent.

[00arthur00]

There's no button for me to reopen the PR.

[squat]

doesnt matter too much one way or the other. I like to use chans if the use-case is simple enough and allows for it, e.g. here. Context is nice for when we have to propagate and build more context that can be cancelled by a parent.

[bwplotka]

Let's keep it simple then.

Thanks @00arthur00 anyway for this! I think building context for propagation cases, sounds like a good rule.