-
Notifications
You must be signed in to change notification settings - Fork 123
add simple pagination to the ListSwaps command #912
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
Conversation
|
I'm going to start looking at some tests now, but wanted to open the PR to get some initial feedback on the approach first. |
0c64b70 to
e3913d6
Compare
|
Added the test which tests the behavior of both the filtering and the pagination. One thing I am still trying to decide on is when starting Maybe return an error instead of just having weird behavior? |
bhandras
left a comment
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.
Concept ACK! Thank you @kornpow, nice addition! Left a few comments.
d1c2e26 to
34c9bc6
Compare
|
Updated the response a bit. Also, now we only unmarshal the swaps we intend to return! |
I can't find where |
starius
left a comment
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.
Nice improvement! 🎉
My main comment is that the order in the map is not stable and offsets themselves are not stable. I propose to use a timestamp instead of an offset and to use an opaque pagination tokens for future compatibility. Also to order the swaps by the initiation time to make the order predictable.
34c9bc6 to
c4d6632
Compare
I have returned |
|
The last test case I am not super satisfied with is: |
|
What do you think about switching from indices to timestamps as page identifiers? Timestamps do not depend on filtering, so this would work better in case some swaps appear or disappear in the middle of iteration. |
c4d6632 to
637395c
Compare
Similar to the API for Timestamps do depend on filtering though, its just filtering based on a timestamp? |
|
I suggest replacing Unlike indices, timestamps remain stable even if some swaps are added or removed from the dataset. Index shifts can lead to duplicates being returned. Using timestamps avoids that issue. |
8e8cd08 to
b25d13b
Compare
|
I did something a little different just to try it out. I can also move to something closer to what you specified @starius if you like. I kept the paging the same, but added a Nanosecond unix timestamp fields to the filter object. So now we can filter on start/end time. Since we are ordering based on |
|
I think, |
e460fa6 to
7020031
Compare
|
Here is the current state. It is getting closer. I appreciate your feedback @starius on the API design. You've definitely helped make it a lot simpler. Here is an example of paging through.
|
d1f3e49 to
29d36e3
Compare
starius
left a comment
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, pending comments 🏆
49b8381 to
75970e3
Compare
starius
left a comment
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!
Added few comments.
75970e3 to
44520de
Compare
44520de to
5f71ed8
Compare
|
@bhandras: review reminder |
This commit adds the ability to set a max_swaps and an index_offset flag to the ListSwaps command. These new fields are applied AFTER the initial filtering step. The response also now passes additional information about the index count and total swaps filtered.
5f71ed8 to
6b3f270
Compare
|
I'm sorry for the delay, and all the churn. It should now properly pass the lint test. |
bhandras
left a comment
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, nice work @kornpow 🎉
This commit adds the ability to set a
max_swapsand anindex_offsetflag to the ListSwaps command. These new fields are applied AFTER the initial filtering step. The response also now passes additional information about thelast_index_offsetcount andtotal_filtered_swaps, which can be used together with the--max_swapsandindex_offsetflags to iterate over your set of swaps in chunks.$ loop listswaps --loop_out_only --max_swaps 1 --index_offset 3Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixesCloses issues
This will close #797