[#1428] fix(server): fallback invalid when local storage can't write#1429
[#1428] fix(server): fallback invalid when local storage can't write#1429zuston merged 4 commits intoapache:masterfrom
Conversation
…ld to avoid event being discarded
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1429 +/- ##
============================================
+ Coverage 53.38% 54.64% +1.26%
+ Complexity 2729 2240 -489
============================================
Files 422 346 -76
Lines 24046 15606 -8440
Branches 2051 1431 -620
============================================
- Hits 12836 8528 -4308
+ Misses 10412 6601 -3811
+ Partials 798 477 -321 ☔ View full report in Codecov by Sentry. |
|
PTAL @leixm @jerqi . I think this is a critical bug in 0.8 branch. |
|
I can't get the point of this pr. |
Please see the test case |
|
cc @xianjingfeng If you have time, could you help review this ? |
| .checkValue( | ||
| ConfigUtils.NON_NEGATIVE_LONG_VALIDATOR, " fallback times must be non-negative") | ||
| .defaultValue(0L) | ||
| .defaultValue(-1L) |
There was a problem hiding this comment.
- What if the users set it to 0.
- If
!storageManager.canWrite(flushEvent), we should ignore retry times. So should we add a new argument namedforcetoorg.apache.uniffle.server.storage.AbstractStorageManagerFallbackStrategy#tryFallbackfor ignoring retry times. WDYT? @zuston @jerqi
| public static final ConfigOption<Long> FALLBACK_MAX_FAIL_TIMES = | ||
| ConfigOptions.key("rss.server.hybrid.storage.fallback.max.fail.times") | ||
| .longType() | ||
| .checkValue( |
There was a problem hiding this comment.
The negative check is unnecessary
There was a problem hiding this comment.
Why do we remove this check?
There was a problem hiding this comment.
if -1，it means we could fallback directly once failed at the first time.
There was a problem hiding this comment.
What about the behaviour of 0?
There was a problem hiding this comment.
For the first time fallback, it will reject for 0
zuston
changed the title
|
PTAL again @xianjingfeng |
|
Merged. Thanks @xianjingfeng @jerqi |
4 participants
What changes were proposed in this pull request?
allow specifying negative fallback threshold to avoid event being discarded
Why are the changes needed?
Fix: #1428
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Unit tests