-
Notifications
You must be signed in to change notification settings - Fork 331
Pagination: Fixing paginate for /api/v1/superuser/logs API (PROJQUAY-5360) #2006
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
data/model/modelutil.py
Outdated
"start_index": start_index, | ||
"page_number": page_number + 1 if page_number else 1, | ||
"is_datetime": is_datetime, | ||
"use_offset": use_offset, |
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.
Do you really need use_offset
, or just to set offset_val
?
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, I can just use offset_val
data/model/modelutil.py
Outdated
""" | ||
results = list(query) | ||
page_token = None | ||
offset_val = 0 |
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.
This is already defined as a default parameter
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, removed.
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
/cherrypick redhat-3.8 |
@Sunandadadi: new pull request created: #2010 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherrypick redhat-3.9 |
@Sunandadadi: new pull request created: #2011 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
* Pagination: Fixing paginate for /api/v1/superuser/logs API * using offset to fetch next page items * adding datetime parsing * using black to format code * removing use_offset * Removing redundant declaration
* Pagination: Fixing paginate for /api/v1/superuser/logs API * using offset to fetch next page items * adding datetime parsing * using black to format code * removing use_offset * Removing redundant declaration
Superuser logs are sorted on datetime field. The next page token contains the value of the start_index of the datetime field for the next page. datetime column on the LogEntry table is not unique, so in some cases, multiple entries can have the same datetime field.
For example, logs have a page limit of 20 entries. If 25 entries have the same datetime - d1, the start_index of the next page is again d1. With the current logic, the same query is run for the next page too. This PR addresses such issues and adds an offset in such cases where (the first 20 entries of page 1 and 1 or more entries of page 2 have the same datetime).