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

Terminate timeloop faster for m3 #185

Merged
merged 7 commits into from
Oct 10, 2022
Merged

Terminate timeloop faster for m3 #185

merged 7 commits into from
Oct 10, 2022

Conversation

artms
Copy link
Contributor

@artms artms commented Oct 7, 2022

We quickly start and stop process (it is wrapper for another command) and timeloop potentially can delay process shutdown by up-to 100ms (_timeResolution). Instead of just sleeping we as well wait for signal that shutdown is in progress and quickly terminate loop.

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2022

CLA assistant check
All committers have signed the CLA.

@artms artms changed the title Terminate timeloop faster Terminate timeloop faster for m3 Oct 7, 2022
Copy link

@rabbbit rabbbit left a comment

Choose a reason for hiding this comment

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

Thanks!

(you probably need another review though)

m3/reporter.go Outdated Show resolved Hide resolved
@artms
Copy link
Contributor Author

artms commented Oct 10, 2022

Production tests fail with errors that timeLoop goroutine is leaking, investigating, I suspect that production is not always properly discarding tally

@artms
Copy link
Contributor Author

artms commented Oct 10, 2022

So it seems code is correct, indeed there are a lot of leaking goroutines because tally reporter is not properly closed. So this change potentially can introduce regression for tests which try to detect leaking goroutines.

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

Merging #185 (4d10933) into master (86b6b09) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   47.31%   47.36%   +0.04%     
==========================================
  Files          64       64              
  Lines        5932     5937       +5     
==========================================
+ Hits         2807     2812       +5     
  Misses       2685     2685              
  Partials      440      440              
Impacted Files Coverage Δ
m3/reporter.go 92.83% <100.00%> (+0.09%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

m3/reporter.go Outdated Show resolved Hide resolved
Copy link
Member

@mway mway left a comment

Choose a reason for hiding this comment

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

Thanks! Just fix lint and then lgtm.

@mway mway merged commit 734a7ff into uber-go:master Oct 10, 2022
brawndou pushed a commit to brawndou/tally that referenced this pull request Nov 17, 2022
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