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

Targetmanager doesn't need timer reset #114

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Targetmanager doesn't need timer reset #114

merged 1 commit into from
Feb 28, 2024

Conversation

puffitos
Copy link
Collaborator

Motivation

The reset timers are unnecessary. The timers carry on automatically and there is no need to reset them.

Changes

Removed the reset call from the timers.

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

Signed-off-by: Bruno Bressi <bruno.bressi@telekom.de>
@puffitos puffitos added the area/target-manager Issues/PRs related to the TargetManager label Feb 28, 2024
@puffitos puffitos self-assigned this Feb 28, 2024
Copy link
Collaborator

@niklastreml niklastreml left a comment

Choose a reason for hiding this comment

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

LGTM

@puffitos puffitos merged commit 532f07d into main Feb 28, 2024
11 checks passed
@lvlcn-t lvlcn-t deleted the fix/remove-reset branch February 28, 2024 13:49
@lvlcn-t
Copy link
Collaborator

lvlcn-t commented Feb 29, 2024

@puffitos Are you sure this was correct? We have timers here, not tickers.

Each operation (refreshing targets, registering, and updating) now only executes once after startup. After that, the select statement blocks indefinitely because none of the channels (checkTimer.C, registrationTimer.C, updateTimer.C) receive any more values because of the expired timer.

As you can see in these logs, each operation is now only triggered once:

$ go run main.go run --config .tmp/sparrow.yaml 
Using config file: .tmp/sparrow.yaml
{"time":"2024-02-29T23:53:22.176459468+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/cmd.NewCmdRun.run.func1","file":"/home/tom/dev/sparrow/cmd/run.go","line":81},"msg":"Running sparrow"}
{"time":"2024-02-29T23:53:22.176503872+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/targets.(*manager).Reconcile","file":"/home/tom/dev/sparrow/pkg/sparrow/targets/manager.go","line":82},"msg":"Starting target manager reconciliation"}
{"time":"2024-02-29T23:53:22.176571169+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/api.(*api).Run.func1","file":"/home/tom/dev/sparrow/pkg/api/api.go","line":76},"msg":"Serving Api","addr":":8080"}
{"time":"2024-02-29T23:53:33.501372239+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/git.(*Client).CommitFile","file":"/home/tom/dev/sparrow/pkg/sparrow/git/git.go","line":121},"msg":"File committed and pushed","file":"tom.dev.sparrow.com.json"}
{"time":"2024-02-29T23:53:52.194231807+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}
{"time":"2024-02-29T23:54:22.194857223+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}
{"time":"2024-02-29T23:54:22.424555542+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/git.(*Client).FetchFiles","file":"/home/tom/dev/sparrow/pkg/sparrow/git/git.go","line":82},"msg":"Successfully fetched all target files","files":1}
{"time":"2024-02-29T23:54:23.437925861+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/sparrow/git.(*Client).CommitFile","file":"/home/tom/dev/sparrow/pkg/sparrow/git/git.go","line":121},"msg":"File committed and pushed","file":"tom.dev.sparrow.com.json"}
{"time":"2024-02-29T23:54:52.195589687+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}
{"time":"2024-02-29T23:55:22.21459645+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}
{"time":"2024-02-29T23:55:52.225627703+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}
{"time":"2024-02-29T23:56:22.242718584+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}
{"time":"2024-02-29T23:56:52.243342933+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}
{"time":"2024-02-29T23:57:22.264238966+01:00","level":"INFO","source":{"function":"github.com/caas-team/sparrow/pkg/config.(*FileLoader).Run","file":"/home/tom/dev/sparrow/pkg/config/file.go","line":91},"msg":"Successfully got local runtime configuration"}

@puffitos
Copy link
Collaborator Author

puffitos commented Mar 1, 2024

Thanks, I'll change those to tickers. This is the price of trusting your tests too much...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/target-manager Issues/PRs related to the TargetManager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants