Skip to content

fix(es): use long-polling to watch reindex tasks#34

Merged
rerowep merged 1 commit into
rero:stagingfrom
rerowep:wep-fix-watch_reindex_task
Jul 6, 2026
Merged

fix(es): use long-polling to watch reindex tasks#34
rerowep merged 1 commit into
rero:stagingfrom
rerowep:wep-fix-watch_reindex_task

Conversation

@rerowep

@rerowep rerowep commented Jul 1, 2026

Copy link
Copy Markdown
Contributor
  • Replace fixed-interval sleep polling with ES long-polling (wait_for_completion + timeout) to cut down on request overhead
  • Add _log_task_progress helper for consistent progress reporting
  • Treat NotFoundError and closed-index RequestError as task completion, and timeout TransportError as an expected retry signal
  • Include the index name in update-mapping error messages
  • Rename --inplace short option from -i to -p to avoid clashing with --interval on the same command

@rerowep
rerowep marked this pull request as ready for review July 1, 2026 08:57
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The Elasticsearch reindex task watcher was rewritten to use server-side long-polling via wait_for_completion with a timeout instead of manual sleep-based polling, with new exception handling for NotFoundError, RequestError, and TransportError. A new helper logs task progress with created/total counts. Error output in update_mapping now includes the target index name. The rebuild command's --inplace short flag changed from -i to -p.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Changes

File Summary
rero_invenio_base/cli/es/index.py Refactored task-watch polling to long-polling with new exception handling; added _log_task_progress helper; included index name in update_mapping error output; changed --inplace short flag from -i to -p.

Related issues: None found.

Related PRs: None found.

Suggested labels: enhancement, cli

Suggested reviewers: None available from provided context.

🐰 A rabbit watched a task take flight,
No more sleepy polls through the night,
It waits and it logs,
Through timeouts and fogs,
Then hops off with the index name in sight!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: switching reindex task watching to long-polling.
Description check ✅ Passed The description accurately and specifically describes the changes in the pull request.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
rero_invenio_base/cli/es/index.py (1)

31-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add missing Sphinx return metadata.

The new docstrings omit :rtype:; _log_task_progress also omits :returns:. As per coding guidelines, use Sphinx-style docstrings with :param x:, :returns:, :rtype: format. <coding_guidelines>

Proposed fix
 def _log_task_progress(task, count):
     """Fetch current task status and print a one-line progress update.

     :param task: task id.
     :param count: elapsed seconds to display.
+    :returns: None.
+    :rtype: None
     """
@@
     :param interval: seconds to wait per poll (passed as timeout to the tasks API).
     :param verbose: print task progress on each poll.
     :returns: True on success, False if the task reported failures.
+    :rtype: bool
     """
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rero_invenio_base/cli/es/index.py` around lines 31 - 54, The docstrings for
_log_task_progress and _watch_reindex_task are missing required Sphinx return
metadata. Update these docstrings to include :returns: and :rtype: entries,
keeping the existing :param annotations, so the function contracts match the
coding guidelines and are consistent with other CLI helpers in index.py.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rero_invenio_base/cli/es/index.py`:
- Around line 37-44: The progress update in the task-watching block is too broad
because the exception handler in the CLI task polling logic swallows all errors
and can hide unrelated bugs. Update the try/except around
current_search_client.tasks.get in the watch command to catch TransportError
instead of Exception, since NotFoundError and RequestError already fall under
that hierarchy, and keep the fallback click.secho path only for those
Elasticsearch transport failures.
- Around line 269-270: The current broad except in the Elasticsearch index
command catches all exceptions, but this block should only handle
client/index-operation failures. Narrow the handler around the Elasticsearch
operation in the CLI flow so it catches the specific Elasticsearch client
exception type(s) used by the index action, while leaving unrelated bugs to fail
normally; keep the existing click.secho error reporting for those targeted
failures.

---

Nitpick comments:
In `@rero_invenio_base/cli/es/index.py`:
- Around line 31-54: The docstrings for _log_task_progress and
_watch_reindex_task are missing required Sphinx return metadata. Update these
docstrings to include :returns: and :rtype: entries, keeping the existing :param
annotations, so the function contracts match the coding guidelines and are
consistent with other CLI helpers in index.py.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: be2201e6-8308-420d-a684-91a2743e2fa7

📥 Commits

Reviewing files that changed from the base of the PR and between 8367018 and 3d26128.

📒 Files selected for processing (1)
  • rero_invenio_base/cli/es/index.py

Comment thread rero_invenio_base/cli/es/index.py
Comment thread rero_invenio_base/cli/es/index.py Outdated
* Replace fixed-interval sleep polling with ES long-polling
  (wait_for_completion + timeout) to cut down on request overhead
* Add _log_task_progress helper for consistent progress reporting
* Treat NotFoundError and closed-index RequestError as task
  completion, and timeout TransportError as an expected retry signal
* Include the index name in update-mapping error messages
* Rename --inplace short option from -i to -p to avoid clashing with
  --interval on the same command

Co-Authored-by: Peter Weber <peter.weber@rero.ch>
@rerowep
rerowep force-pushed the wep-fix-watch_reindex_task branch from 3d26128 to 1b95acb Compare July 1, 2026 09:24
@rerowep
rerowep merged commit bb7547e into rero:staging Jul 6, 2026
7 checks passed
@rerowep
rerowep deleted the wep-fix-watch_reindex_task branch July 6, 2026 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants