Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Handle all exceptions during queue processing #591

Conversation

lenaschoenburg
Copy link
Contributor

Fixes #590

This reverts a change that made QueueProcessor only handle InterruptedException. Instead we want to ignore and log all exceptions because otherwise the QueueProcessor thread would fail silently.

This reverts a change that made QueueProcessor only handle
InterruptedException. Instead we want to ignore and log all exceptions
because otherwise the QueueProcessor thread would fail silently.

Signed-off-by: Ole Krüger <okrueger@cognesys.de>
@lenaschoenburg lenaschoenburg force-pushed the handle-queue-processor-exceptions branch from c480b70 to 6319229 Compare February 22, 2019 15:12
@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #591 into master will decrease coverage by 0.25%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #591      +/-   ##
============================================
- Coverage     89.53%   89.27%   -0.26%     
+ Complexity      542      540       -2     
============================================
  Files            68       68              
  Lines          1949     1949              
  Branches        251      251              
============================================
- Hits           1745     1740       -5     
- Misses          129      133       +4     
- Partials         75       76       +1
Impacted Files Coverage Δ Complexity Δ
...egertracing/internal/reporters/RemoteReporter.java 82.55% <0%> (ø) 7 <0> (ø) ⬇️
...rtracing/internal/reporters/CompositeReporter.java 71.42% <0%> (-28.58%) 6% <0%> (-1%)
...gertracing/internal/reporters/LoggingReporter.java 81.81% <0%> (-9.1%) 4% <0%> (-1%)

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 c88d448...6319229. Read the comment docs.

@lenaschoenburg
Copy link
Contributor Author

I'm not sure this needs tests. If it does I'd be grateful for some suggestions on how to add a meaningful test.

@yurishkuro
Copy link
Member

I'll merge, but if you'd like to add a test it would prevent the regression in the future. The easiest way to do that is by using a mock Sender that throws an exception.

@yurishkuro yurishkuro merged commit 167ecd6 into jaegertracing:master Feb 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants