Reindex spaces concurrently - #3207
Conversation
This speeds up reindexing all spaces which happend sequentially space after space until now. The level of concurrency can be configured using the SEARCH_REINDEX_CONCURRENCY env var (3 by default).
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
🟢 Coverage 0.00% diff coverage · -0.02% coverage variation
Metric Results Coverage variation ✅ -0.02% coverage variation (-1.00%) Diff coverage ✅ 0.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (bb8cefa) 83760 19256 22.99% Head commit (d50d8d8) 83772 (+12) 19244 (-12) 22.97% (-0.02%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3207) 16 0 0.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Two findings:
-
Should we just log the indexing errors in the server and otherwise swallow them?
I would probably prefer to collect them and report to the caller (the cli) that indexing (some) spaces failed.. Reporting the actual spaces in a structured format would probably mean touching the proto files. If you want to avoid that, a simple summary of failed spaces in a single error message would still be better than nothing imho. -
It might be more complex and maybe not necessary right now, but I could imagine that an operator might want to set the concurrency value depending on the load of the cluster without restarting the search service ...
Maybe let's name the option_MAX_CONCURRENCYso we can introduce a cli arg later?
This would obviously need a proto change as well
Yes you're right, the experience when using the CLI needs to be improved. We also still have the problem that the CLI doesn't wait indefinitely until the reindex process has completed, so returning an error message usually wouldn't reach the user (See #2592). I would defer that to a proper fix alongside #2592 and just go with the server log messages for now.
Good point, I'll rename the option as suggested. |
|
Sounds reasonable, thanks! |
This speeds up reindexing all spaces which happend sequentially space after space until now. The level of concurrency can be configured using the SEARCH_REINDEX_CONCURRENCY env var (3 by default).