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

Fix shutdown order for collector components #2381

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

Sreevani871
Copy link
Contributor

@Sreevani871 Sreevani871 commented Aug 10, 2020

bug fix: Sequence of shutdown calls resulting in crash due to panic and leads to span loss to avoid span loss

Which problem is this PR solving?

Short description of the changes

  • Changes include reordering of the shutdown sequence as follows to allow collector to stop accepting new spans and drain the queue to avoid span loss.
    servers -> queue processors (with drain) -> writers

@Sreevani871 Sreevani871 requested a review from a team as a code owner August 10, 2020 17:03
…ose to avoid span loss

Signed-off-by: Sreevani871 <sreevani@freshdesk.com>
@yurishkuro yurishkuro changed the title bug fix: Sequence of shutdown calls resulting in crash due to panic and leads to span loss Fix shutdown order for collector components Aug 11, 2020
@codecov
Copy link

codecov bot commented Aug 11, 2020

Codecov Report

Merging #2381 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2381   +/-   ##
=======================================
  Coverage   95.60%   95.61%           
=======================================
  Files         206      206           
  Lines       10548    10548           
=======================================
+ Hits        10084    10085    +1     
+ Misses        396      395    -1     
  Partials       68       68           
Impacted Files Coverage Δ
cmd/flags/admin.go 77.77% <0.00%> (-1.59%) ⬇️
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afe491f...587f664. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM!

@jpkrohling jpkrohling merged commit 4343528 into jaegertracing:master Aug 11, 2020
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.

Collector: Sequence of shutdown calls resulting in crash due to panic and leads to span loss
2 participants