Validate revisions between progress notify #17810
Conversation
|
Skipping CI for Draft Pull Request. |
serathius
force-pushed
the
robustness-revisions-between-progress
branch
2 times, most recently
from
7029f80 to
8eb47aa
Compare
serathius
force-pushed
the
robustness-revisions-between-progress
branch
3 times, most recently
from
7671845 to
cb76098
Compare
|
Marking as draft found a new edge case where request revision can be higher than bookmark. |
serathius
force-pushed
the
robustness-revisions-between-progress
branch
from
cb76098 to
50ecda4
Compare
| err = validateBookmarkable(lg, r) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if eventHistory != nil { |
There was a problem hiding this comment.
although it is trivial, can you add a test case where eventHistory is empty but there are multiple progress notify events?
There was a problem hiding this comment.
A passing case or a failing one?
There was a problem hiding this comment.
a passing case. And the test case for 2 progressNotify responses with the wrong rev order is also missing.
serathius
force-pushed
the
robustness-revisions-between-progress
branch
3 times, most recently
from
327987a to
ad23f87
Compare
|
/retest |
Simplifying bookmarkable to just validate revision order between events and progress notifies. Use reliable to validate if events are missing, but still report broken resumable if first event after revision is missing. It's easier to have one place that validates event slices. Signed-off-by: Marek Siarkowicz <[email protected]>
serathius
force-pushed
the
robustness-revisions-between-progress
branch
from
ad23f87 to
964680c
Compare
|
defer to @MadhavJivrajani @siyuanfoundation @ArkaSaha30 @fuweid to review robustness test PRs. |
| if index == len(events) { | ||
| continue | ||
| for _, watch := range report.Watch { | ||
| firstRev := firstExpectedRevision(watch) |
There was a problem hiding this comment.
is it possible to have firstRev > lastRev when there is only one response and it's progress notification?
There was a problem hiding this comment.
Yes, It's covered in test Bookmarkable - progress notification lower than watch request - pass.
Client can request to watch from higher revision than clusters is on. This means that progress notifies will come will lower revision than watch request.
Validate delivery of events between progress notifies. #17771 prevented empty streams for revision ranges between watch requiring revision and progress notify. This missed one more case, ranges between two progress notifies. This PR extends reliable validation to validate whole ranges.
Simplifying bookmarkable to just validate revision order between events
and progress notifies.
Use reliable to validate if events are missing, but still report
broken resumable if first event after revision is missing. It's easier
to have one place that validates event slices.