ranking: remove old code path#57468
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff c70295c...276f08f.
|
|
|
||
| - | ||
| - The following experimental settings in site-configuration have not been used by the backend since version 5.1 and are now deprecated: `maxReorderQueueSize`, `maxQueueMatchCount`, `maxReorderDurationMS`. [#57468](https://github.com/sourcegraph/sourcegraph/pull/57468) | ||
| - The feature-flag `search-ranking`, which allowed to disable the improved ranking introduced in 5.1, is now deprecated. [#57468](https://github.com/sourcegraph/sourcegraph/pull/57468) |
There was a problem hiding this comment.
you may want to rebase first, I think a new section in the changelog was recently added for 5.2.1 that git merge logic may decide to move this into.
Should we mention setting them is a noop now? Deprecated normally implies we will remove them but they still work.
| if opts == nil { | ||
| return &nopFlushCollector{sender} | ||
| } |
There was a problem hiding this comment.
maybe keep the doc string on why we allow nil opts? Can we just get rid of this.
There was a problem hiding this comment.
The searcher interface allows nil options which I believe is the root cause of the problem. Apart from tests, there are call sites in "insights" which call with nil options. I will put the doc string back.
| metricReorderQueueSize.WithLabelValues().Observe(float64(rq.metricMaxLength)) | ||
| metricMaxMatchCount.WithLabelValues().Observe(float64(rq.metricMaxMatchCount)) | ||
| metricFinalQueueSize.Add(float64(rq.queue.Len())) | ||
| metricMaxSizeBytes.WithLabelValues().Observe(float64(rq.metricMaxSizeBytes)) |
There was a problem hiding this comment.
I think we have dashboards on these? Need to update them
The enhanced ranking logic is GA since 5.1. So far we haven't had any issue so we should remove the old code path with 5.3. Test plan: CI
There are no dashboards connected to these metrics
35fb7a5 to
910b4a5
Compare
| searchOpts.DocumentRanksWeight = conf.SearchDocumentRanksWeight() | ||
| } | ||
| // This enables the use of document ranks in scoring, if they are available. | ||
| searchOpts.UseDocumentRanks = true |
There was a problem hiding this comment.
I assume we're not removing the repo-based ranking code path from Zoekt (because there are other users/ clients depending on that)?
There was a problem hiding this comment.
I didn't plan to change anything in Zoekt. I assume most if not all other clients don't use document ranks.
The enhanced ranking logic is GA since 5.1. So far we haven't had any issue so we should remove the old code path with 5.3.
Test plan:
CI