Adapt rebuild-index to the server now doing the complete rebuild itself, including the hot swap - #311
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the qlever rebuild-index client command to match the new server-side rebuild-and-swap protocol by delegating the rebuild work to the server, streaming the rebuild log while it runs, and optionally performing a post-swap test query.
Changes:
- Remove all client-side index directory creation/moving/validation logic and replace it with a single
cmd=rebuild-indexserver request. - Add live rebuild-log following while the HTTP request is in progress and report the server-returned
previous-index-dirafter completion. - Adjust CLI surface: remove now-obsolete options, add
--rebuild-tmp-dir,--rebuild-previous-index-dir,--keep-previous-index-dirsdefaulting toall, and--test-query.
Comments suppressed due to low confidence (3)
src/qlever/commands/rebuild_index.py:87
rebuild_index_cmdis built by string-interpolating user-controlled values (endpoint, tmp/previous dirs, access token) directly into a shell command and uses-dinstead of--data-urlencode. This breaks for values containing spaces or&/=and also creates shell-injection risk becauserun_commandexecutes viabash -c.
Prefer --data-urlencode with shlex.quote for each key=value argument, and quote the endpoint as well.
rebuild_index_cmd = (
f"curl -s -w '\\n%{{http_code}}' {args.host_name}:{args.port} "
f"-d cmd=rebuild-index"
)
if args.rebuild_tmp_dir is not None:
src/qlever/commands/rebuild_index.py:116
tail_cmdis executed withshell=Trueand includeslog_file_glob, which is derived fromargs.rebuild_tmp_dirandargs.name. This is vulnerable to shell injection and also breaks on whitespace/shell metacharacters in paths or names.
Consider avoiding shell=True entirely (compute the log path in Python and run tail with an argv list), or strictly validate/escape these inputs before interpolating.
log_file_glob = (
args.rebuild_tmp_dir
if args.rebuild_tmp_dir is not None
else "rebuild.*.tmp"
) + f"/{args.name}.rebuild-index-log.txt"
tail_cmd = (
f"while true; do LOG_FILE=$(ls -t {log_file_glob} 2> /dev/null "
f'| head -1); [ -n "$LOG_FILE" ] && break; sleep 0.1; done && '
f'exec tail -f "$LOG_FILE"'
)
tail_proc = subprocess.Popen(tail_cmd, shell=True)
src/qlever/commands/rebuild_index.py:183
- The code claims to sort
previous.*directories “by creation time”, but usesst_ctime, which is not creation time on many Unix filesystems (it’s inode change time and can change on chmod/chown). This can lead to unexpected retention/deletion ordering.
Use st_mtime (modification time) or parse the timestamp from the directory name for a stable ordering.
# Clean up previous index directories according to
# `--keep-previous-index-dirs`. Find all subdirectories starting with
# `previous.`, ordered from oldest to newest (by creation time), and
# keep or delete them according to the specified policy.
if args.keep_previous_index_dirs != "all":
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import json | ||
| import shutil | ||
| import socket | ||
| import subprocess | ||
| import time |
rebuild-index to the server-side rebuild protocolrebuild-index to the server now doing the complete rebuild itself, including the hot swap
Since ad-freiburg/qlever#2832, the QLever server performs the complete index rebuild itself: it builds the new index in a temporary directory (from the current data, including updates), moves the current index to a
previous.<datetime>directory, moves the new index into its place, and swaps it in without a restart.This change adapts
qlever rebuild-indexaccordingly: the command now simply triggers the rebuild viacmd=rebuild-indexand waits for it to finish, showing the rebuild log while it is running and reporting the directory of the previous index afterwards. The default is a plainqlever rebuild-indexwith no arguments, which does the following: the server builds the new index inrebuild.<timestamp>.tmp, hot-swaps it in (queries and updates keep running throughout, without a restart), moves the new index files to the base directory, and moves the old index toprevious.<timestamp>.The options
--rebuild-tmp-dirand--rebuild-previous-index-dirare passed through to the eponymous parameters of the server command. The option--keep-previous-index-dirscleans up oldprevious.*directories after a successful rebuild (choices:all,none,original-only,most-recent-only,original-and-most-recent; the default isoriginal-and-most-recent, as before). With the option--test-query, the command fails if a simple query to the server after the swap does not succeed.NOTE: All the moving machinery that the
qlever rebuild-indexcommand itself implemented so far (the server only built the new index, the command then moved the directories around) is gone, along with the options that only existed to configure it, and so is the validation server that was previously started on the new index before moving it into place (with the server-side swap, there is no pre-swap moment left in which a separate server could probe the new index;--test-queryis the post-swap replacement).