This repository was archived by the owner on Sep 30, 2024. It is now read-only.
fix: Check (uploadID, path) pairs in bulk#63485
Merged
Merged
Conversation
varungandhi-src
commented
Jun 26, 2024
| AssociatedIndexID *int `json:"associatedIndex"` | ||
| } | ||
|
|
||
| var _ core.UploadLike = &CompletedUpload{} |
Contributor
Author
There was a problem hiding this comment.
I changed this because the other code currently works with []CompletedUpload, and I wanted to use that as []U where U implements UploadLike.
varungandhi-src
commented
Jun 26, 2024
| // The skipDBCheck function avoids consulting the database for certain uploads. | ||
| // | ||
| // The order of the returned slice matches the order of candidates. | ||
| func filterUploadsImpl[U core.UploadLike]( |
Contributor
Author
There was a problem hiding this comment.
This function is generic because:
- Reading and typing
uploadsshared.CompletedUploadin multiple places gets old quickly. - There's actually no need for the other fields, we only really care about the ID and the Root.
kritzcreek
suggested changes
Jun 27, 2024
kritzcreek
left a comment
Contributor
There was a problem hiding this comment.
Thank you for looking into this. Have a few comments (mostly docs related)
d9c3112 to
e6403e7
Compare
kritzcreek
approved these changes
Jun 27, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes https://linear.app/sourcegraph/issue/GRAPH-700
Previously, there were two code paths which were checking if certain
upload IDs containing a specific repo-relative path. One was using the
better GetPathExists API, the another one that I recently added was
much more stupid, it was deserializing the full SCIP Document.
However, using
GetPathExistsin a loop is also not great; we shouldpush the loop into the database. This patch replaces both of those usage
sites with a single function that handles bulk filtering, and replaces
the
GetPathExistslsifstore method withFindDocumentIDs.Q: Why are we returning document IDs if the caller doesn't need them?
A: Getting the document ID is essentially "free", so I thought it would be
better to return a mapping instead of a set.
However, I didn't generalize the API to support multiple paths within
the same upload, as that would complicate the method signature somewhat.
(the argument and return types would be both
collections.Set[Pair[int, UploadRelPath]]).I'm open to changing the method to work with sets instead and renaming
it to
CheckPathsExist.Test plan
Modified GetPathExists test to use the new FindDocumentIDs API.
Changelog