-
Notifications
You must be signed in to change notification settings - Fork 89
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
chore: remove unused feature "document ranks" #842
Conversation
Document ranks was an experimental feature that allowed Zoekt to query Sourcegraph for additional ranking signals for files at index-time. We have decided to remove this feature because it was never used in production. Note: Most of the changes are Sourcegraph specific. However there are some exported fields in `SearchOptions` and `build.Options` as well as flags for zoekt-git-index which I simply removed. Test plan: updated tests
@@ -13,9 +13,6 @@ service ZoektConfigurationService { | |||
// List returns the list of repositories that the client should index. | |||
rpc List(ListRequest) returns (ListResponse) {} | |||
|
|||
// DocumentRanks returns the rank vectors for all documents in the specified repository. | |||
rpc DocumentRanks(DocumentRanksRequest) returns (DocumentRanksResponse) {} |
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 has not been called for a long time, so it should be save to remove.
offlineRanking := flag.String("offline_ranking", "", "the name of the file that contains the ranking info.") | ||
offlineRankingVersion := flag.String("offline_ranking_version", "", "a version string identifying the contents in offline_ranking.") |
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 backward incompatible but I believe it's very unlikely that anyone used this outside Sourcegraph.
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 clean-up. To check I understand, this will not trigger a reindex for everyone right? Because customers had an empty DocumentRanksVersion
all along (we never enabled the feature for them).
@@ -386,11 +386,6 @@ nextFileMatch: | |||
// Update stats based on work done during document search. | |||
updateMatchTreeStats(mt, &res.Stats) | |||
|
|||
// If document ranking is enabled, then we can rank and truncate the files to save memory. | |||
if opts.UseDocumentRanks { |
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 seems weird to me. Can't we always sort and truncate the files here?
closing this in favor of #853 |
Document ranks was an experimental feature that allowed Zoekt to query Sourcegraph for additional ranking signals for files at index-time. We have decided to remove this feature because it was never used in production.
Note:
Most of the changes are Sourcegraph specific. However there are some exported fields in
SearchOptions
andbuild.Options
as well as flags for zoekt-git-index which I simply removed. I believe it is fairly unlikely that those were used by anyone outside Sourcegraph.Test plan:
updated tests