-
Notifications
You must be signed in to change notification settings - Fork 1.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
optimize UX for loading pipeline pages #1085
Conversation
/assign @hongye-sun |
/hold |
backend/api/run.proto
Outdated
@@ -150,7 +150,8 @@ message TerminateRunRequest { | |||
} | |||
|
|||
message ListRunsResponse { | |||
repeated Run runs = 1; | |||
repeated Run runs = 1 [deprecated=true]; |
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.
I think we should hold off on this for now, but you change it if you think it's alright. The Run
(not detail) part of the API is also used for the experiment list page, and I'm not sure yet that we'll always prefer the full run detail. it seems like the core part of this PR is still valuable without this.
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.
reverted.
/hold cancel |
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
@@ -145,21 +145,18 @@ func (s *RunStore) buildSelectRunsQuery(selectCount bool, opts *list.Options, | |||
} | |||
|
|||
sqlBuilder := opts.AddFilterToSelect(filteredSelectBuilder) | |||
if err != nil { |
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.
Why is this removed?
sqlBuilder = s.addResourceReferences(sqlBuilder) | ||
sqlBuilder = opts.AddPaginationToSelect(sqlBuilder) | ||
sqlBuilder = opts.AddSortingToSelect(sqlBuilder) |
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.
Just wonder if it's still required to add sorting query if inner result has already been sorted.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
speed up the query time for list runs
For example:
Before
After
This change is