Skip to content

Remove hardcoded sleep in shutdown for distributor - #3687

Merged
joe-elliott merged 2 commits into
grafana:mainfrom
chodges15:distShutdown
May 21, 2024
Merged

Remove hardcoded sleep in shutdown for distributor#3687
joe-elliott merged 2 commits into
grafana:mainfrom
chodges15:distShutdown

Conversation

@chodges15

@chodges15 chodges15 commented May 18, 2024

Copy link
Copy Markdown
Contributor

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

CLAassistant commented May 18, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@joe-elliott

Copy link
Copy Markdown
Collaborator

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.

@knylander-grafana knylander-grafana left a comment

Copy link
Copy Markdown
Contributor

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 May 21, 2024
@chodges15

Copy link
Copy Markdown
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.

@joe-elliott joe-elliott left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
@chodges15
chodges15 deleted the distShutdown branch May 23, 2024 03:07
mattdurham pushed a commit to mattdurham/tempo that referenced this pull request Jun 18, 2026
Co-authored-by: Joe Elliott <number101010@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants