-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor: Start DRY'ing filtered paginate code #16099
Conversation
@@ -85,6 +85,16 @@ func FilteredPaginate( | |||
end := offset + limit | |||
|
|||
for ; iterator.Valid(); iterator.Next() { | |||
if numHits == end+1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I got this wrong right here, because it looks at iterator.Key()
. (Which is a different value in this loop)
This is a bit annoying to make consistent with the old behavior. Will adjust the de-duplicated loop to get this consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically the old behavior would ideally worked with numHits = end
right here.
But it actually made nextKey skip to the next hit, which is kind of annoying to get to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current PR has this problem fixed.
In a later breaking release when redesigning the Filtered paginate, I would recommend changing this behavior. (As it will both be more correct from a performance expectation perspective, and code quality cleanup. We could delete all the extra for loops everywhere.)
However this PR is fully non-breaking.
Failing tests seem like github issues? (They all say rate limit) All actual go tests still pass |
Ready for review / no more planned updates! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor nit for docs reason. Thanks.
Co-authored-by: testinginprod <[email protected]>
Putting into the merge when ready queue (as its blocking another PR), and is fully compatible / not state machine affecting |
Description
Reduce some of the lines of code in this filtered paginate code. Got linked to it, and got bothered by how much code repetition / extra logic there is.
Feel free to close if not worth the review capacity, or to take over.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
- Not at all user facingReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change