fix: Improve performance for the TreeCommitHistory endpoint#1877
fix: Improve performance for the TreeCommitHistory endpoint#1877alanpeixinho wants to merge 1 commit intokernelci:mainfrom
Conversation
bdd01aa to
3c4f037
Compare
3c4f037 to
a1886f0
Compare
c513c76 to
8db039a
Compare
ade6703 to
1da2177
Compare
MarceloRobert
left a comment
There was a problem hiding this comment.
Some filters are not working [in treeDetails]:
- test hardware - 0 results
- boot origin - not applying
- test inconclusive status - missing data
- boot inconclusive status - missing data
- build inconclusive status - 0 results / missing data
- test issue (uncategorized) - 0 results
- build issue (real) - 0 results
- boot issue (any) - 0 results
I'm also getting error 400 on trees without tree_name or git_branch. Maybe we could just fallback to the old endpoint in those cases.
Also also, the hardwareDetails page requests the commitHistory when there's a single tree selected but it is returning error 400 for me
| git_branch: Optional[str], | ||
| tree_name: Optional[str], |
There was a problem hiding this comment.
tree_name and git_branch can be null but there's no query condition in the query if they are None
There was a problem hiding this comment.
My idea was to not apply filtering when not specified, do we have cases, where we want to search specifically for null branchs and tree names?
There was a problem hiding this comment.
It's not common but there are cases of trees without name/branch. Ex.: broonie tree, even though the current commitHistory is not right here either, it at least lets the user navigate through the commits
There was a problem hiding this comment.
I am adding an option to search for exactly NULL tree names and branches.
What I dislike is that it makes this parameters no longer trully an optional filter, as null is now just another id
There was a problem hiding this comment.
That's how we have to work with this database, unfortunately. I think you added the filter to the get_tree_commit_history_hashes_aggregated query but not for the get_tree_commits query
| if include_boots and not include_tests: | ||
| boot_filter = "\nAND t.path LIKE 'boot%%'" | ||
| elif include_tests and not include_boots: | ||
| boot_filter = "\nAND t.path NOT LIKE 'boot%%'" |
There was a problem hiding this comment.
the boot filter should be = boot or like boot.%. This is because a test called "bootloader_something" is not considered a boot test, only those with the specific name "boot" or "subtests" of boot
There was a problem hiding this comment.
Both conditions should apply at all times, so it should be
if include_boots and not include_tests:
boot_filter = "\nAND (t.path ='boot' OR t.path LIKE 'boot.%%')"
elif include_tests and not include_boots:
boot_filter = "\nAND t.path NOT LIKE 'boot.%%' AND t.path != 'boot'"
There was a problem hiding this comment.
You are absolutely right. My oversight.
There was a problem hiding this comment.
This change is still missing, no?
There was a problem hiding this comment.
Yes, this change was still local. It should have been updated now.
The 400 returns was a choice for empty cases. Is it preferrable to return 200 and an empty list? |
For the cases that I have seen the error 400, they should actually be returning data, not be empty. As for when they should be empty, then I guess we have been using status 200 with an error message, but it can be up for debate again; my preference would be the 200 with "error" message (I know it's not a real error but the field in the response is called |
6c5cd08 to
84b21c7
Compare
84b21c7 to
c2b8765
Compare
* Improve query performance for commit history
* Add TreeCommitsListView and type models
* Update frontend API and CommitNavigationGraph
c2b8765 to
f210cf7
Compare
Summary
TreeCommitHistoryendpoint with improved database queries for better performanceTreeCommitsListViewendpoint for future paginated tree commit listingCommitNavigationGraphcomponent and API integrationHow to Test
Closes #1883