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

Remove hardcoded sleep in shutdown for distributor #3687

Merged
merged 2 commits into from
May 21, 2024

Conversation

chodges15
Copy link
Contributor

@chodges15 chodges15 commented May 18, 2024

What this PR does:
This sleep is being removed in favor of using the configuration item shutdown_delay for generic server shutdown across all Tempo components introduced in #3395.

Which issue(s) this PR fixes:
Fixes #2353

Checklist

  • Tests updated [N/A, existing coverage]
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented May 18, 2024

CLA assistant check
All committers have signed the CLA.

@joe-elliott
Copy link
Member

I had forgotten about this. We semi-recently added a shutdown_delay option that works for all components:

#3395

I am fine with just removing the lines that sleep for 30s. They were not well thought out when they were added, but have never bothered us enough to remove.

Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Approving doc update. Thank you for adding doc update withj your PR!

@knylander-grafana knylander-grafana self-requested a review May 20, 2024 21:59
@chodges15 chodges15 changed the title Add configurable graceful shutdown period for distributor Remove hardcoded sleep in shutdown for distributor May 21, 2024
@chodges15
Copy link
Contributor Author

@joe-elliott that makes way more sense, I should have used git blame to look at the existing shutdown_delay and see it was added somewhat recently. Was really scratching my head as to why Tempo needed two.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@joe-elliott
Copy link
Member

I've been wanting to get rid of this forever and keep forgetting to just remove it. Thanks so much for getting this done!

@joe-elliott joe-elliott merged commit c1a1a29 into grafana:main May 21, 2024
14 checks passed
@chodges15 chodges15 deleted the distShutdown branch May 23, 2024 03:07
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.

Shutting down tempo always takes at least 30 seconds
4 participants