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

Loki: Use a new context to update the ring state after a failed chunk transfer #2330

Merged
merged 3 commits into from
Jul 9, 2020

Conversation

slim-bean
Copy link
Collaborator

Previously the GRPC stream context was used to update the ring state back to PENDING after a failed chunk transfer.
However if the transfer failed because of a GRPC error this context would get canceled and this would prevent Loki from updating the ring state and it would force an os.Exit

This change uses a new context to update the ring state after a failed transfer to avoid this issue and also makes the error message more clear to explain why the loki process exits.

@codecov-commenter
Copy link

Codecov Report

Merging #2330 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2330      +/-   ##
==========================================
- Coverage   61.23%   61.23%   -0.01%     
==========================================
  Files         158      158              
  Lines       12798    12803       +5     
==========================================
+ Hits         7837     7840       +3     
- Misses       4366     4368       +2     
  Partials      595      595              
Impacted Files Coverage Δ
pkg/ingester/transfer.go 63.01% <0.00%> (-2.24%) ⬇️
pkg/promtail/targets/file/tailer.go 76.13% <0.00%> (-2.28%) ⬇️
pkg/promtail/targets/file/filetarget.go 70.48% <0.00%> (+1.80%) ⬆️
pkg/querier/queryrange/downstreamer.go 97.93% <0.00%> (+2.06%) ⬆️

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

slim-bean and others added 2 commits July 9, 2020 15:45
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>
@slim-bean slim-bean merged commit 635e469 into master Jul 9, 2020
@slim-bean slim-bean deleted the fix-context-ring-update-fail branch July 9, 2020 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants