[SDK] Fix forceflush may wait for ever
Fixes #2574
May also fixes #2583
Changes
- Using a sequence to check if a complete notification includes the datas when call
ForceFlush.
For significant contributions please make sure you have completed the following items:
- [ ]
CHANGELOG.mdupdated for non-trivial changes - [x] Unit tests have been added
- [x] Changes in public API reviewed
@ThomsonTan Is there any problem in this PR? Maybe we can close #2553 if this PR is merged. It fixes the same problems as #2553 do and also fix the simular problems in trace and logs.
BTW, can you explain in brief that why the sequence notification can break the potential forever wait? Thanks.
BTW, can you explain in brief that why the sequence notification can break the potential forever wait? Thanks.
When more than one threads call ForceFlush concurrency, is_force_flush_notified may be set true only once in background thread and one of the threads call ForceFlush will wait is_force_flush_notified for ever.
Codecov Report
Attention: Patch coverage is 95.94595% with 3 lines in your changes are missing coverage. Please review.
Project coverage is 87.70%. Comparing base (
497eaf4) to head (d3df9a4). Report is 65 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #2584 +/- ##
==========================================
+ Coverage 87.12% 87.70% +0.58%
==========================================
Files 200 190 -10
Lines 6109 5852 -257
==========================================
- Hits 5322 5132 -190
+ Misses 787 720 -67
| Files | Coverage Δ | |
|---|---|---|
| ...pentelemetry/sdk/logs/batch_log_record_processor.h | 100.00% <100.00%> (ø) |
|
| ...ude/opentelemetry/sdk/trace/batch_span_processor.h | 100.00% <100.00%> (ø) |
|
| sdk/src/logs/batch_log_record_processor.cc | 89.21% <96.16%> (+1.71%) |
:arrow_up: |
| ...metrics/export/periodic_exporting_metric_reader.cc | 81.32% <95.00%> (+1.98%) |
:arrow_up: |
| sdk/src/trace/batch_span_processor.cc | 94.08% <96.16%> (+1.65%) |
:arrow_up: |
Adding ok-to-merge.
Waiting to give @lalitb and @esigo a chance to comment, and planning to merge early next week.