-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: add total_runs in response of GET /runs endpoint #183
base: develop
Are you sure you want to change the base?
feat: add total_runs in response of GET /runs endpoint #183
Conversation
611152b
to
b9f7d0b
Compare
b9f7d0b
to
ea05099
Compare
@Mifrill thanks for the PR! I would be really interested to know your use case and what you are trying to solve with the elasticsearch has the concept of {
"runs": [],
"metadata": {
"total_runs": 5000
}
} |
+1 this seems pretty important for neat pagination |
total_runs: | ||
type: integer | ||
description: >- | ||
Total count of workflow runs that the service has executed or is executing and the caller has permission to see. |
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.
Suggestion for brevity, and future-proofing (in case we ever add the ability to filter results from this endpoint). I'd also suggest replacing the words "workflow runs" with just "runs", per the WES terminology.
Total count of workflow runs that the service has executed or is executing and the caller has permission to see. | |
Total count of runs in the result set. |
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.
in case we ever add the ability to filter results from this endpoint
not sure I follow.
The sentence has executed or is executing
is looks generic in terms of workflow-execution-service-schemas
.
The sentence the caller has permission to see
didn't set any constraints in case of policy scope implementation, the records might be across entire data as well.
"workflow runs" with just "runs", per the WES terminology
As I see in other examples, the workflow runs
preferred:
https://github.com/ga4gh/workflow-execution-service-schemas/blob/develop/openapi/components/schemas/RunId.yaml
https://github.com/ga4gh/workflow-execution-service-schemas/blob/develop/openapi/components/schemas/RunListResponse.yaml
https://github.com/ga4gh/workflow-execution-service-schemas/blob/develop/openapi/components/schemas/RunStatus.yaml
@patmagee thanks for the review!
The total count required to implement "orthodox" pagination, by dividing the total count by page size we can provide a direct link for any page. Rather than in the current implementation only possible to go next page or the previous page.
In my understanding, the implementation of the counter is the responsibility of each particular service, hence the possible performance issues should be handled on the specific service side. There are many possibilities to avoid performing expensive operations like
I don't think it is a good way to go because if we will follow that logic, the filed |
@Mifrill thanks for the clarification. I think your suggestion makes a lot of sense, and I wonder if there is a way to frame this PR in a forward looking way and consider what we would need to improve pagination in general (starting with a total_runs). There is this open issue, but it has not really gotten any steam lately. I know trs (@denis-yuen) has a specific approach that the settled on which is externally documented, so I wonder if we should likewise investigate what paging schemas are there in the wild and what fits with WES in a holistic way |
@Mifrill would you mind updating your changes against the current develop branch? We merged all of the openapi components into a single file to better work with our tooling |
…get-runs-endpoint-response
@patmagee thanks for the ping 👍. Updated |
On this subject, I would like to draw attention to the draft (see link in next comment by @jaeddy) of the upcoming GA4GH pagination style guide (being drafted to address ga4gh/TASC#29). The style guide is not very prescriptive at the moment (I think it's rather a survey of currently used pagination solutions across the GA4GH API landscape with a discussion of pros and cons) and is definitely open for feedback. |
FYI @uniqueg - I believe this is the latest version of the pagination guide from @andrewyatz and @mamanambiya. |
Yes this is the right one |
Thanks for pointing that out @jaeddy 🙏 |
Hey @patmagee, @cjllanwarne just a gentle reminder, please have a look at this PR when you get a time 🙏 |
The property
total_runs
allows to implement the pagination with the references for each page, includes the last page and showing number of pages.Related to #159 (comment)