Skip to content

refactor: Remove redundant retries from batch_add_requests #391

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

Merged
merged 4 commits into from
May 1, 2025

Conversation

Pijukatel
Copy link
Contributor

@Pijukatel Pijukatel commented Apr 24, 2025

Description

Remove retries from batch_add_requests (such retries already exist on the http client level).
Add deprecation warnings when retires related arguments are used.
Align sync and async version to not ignore exceptions from the http client.
Add tests.

Issues

Align sync and async version to both swallow exception, but return unprocessed requets
Add tests
@Pijukatel Pijukatel added debt Code quality improvement or decrease of technical debt. t-tooling Issues with this label are in the ownership of the tooling team. labels Apr 24, 2025
@github-actions github-actions bot added this to the 113rd sprint - Tooling team milestone Apr 24, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Apr 24, 2025
@Pijukatel Pijukatel changed the title refactor!: Remove redundant retires from batch_add_requests refactor!: Remove redundant retries from batch_add_requests Apr 24, 2025
@Pijukatel Pijukatel force-pushed the remove-redundant-retries-from-batch-add-requests branch from 059d5ce to e411ca4 Compare April 24, 2025 14:02
@Pijukatel Pijukatel requested a review from Copilot April 25, 2025 07:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the request queue client by removing redundant retry logic from the batch_add_requests methods (both sync and async) and aligning exception handling to rely on the HTTP client’s built-in retry capabilities. Additionally, new tests have been added to verify that exceptions are raised when appropriate and that partially processed batches are handled correctly.

  • Removed retry logic and associated arguments from batch_add_requests.
  • Aligned sync and async methods to surface HTTP client exceptions.
  • Added unit tests covering exception and partial processing scenarios.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/unit/test_client_request_queue.py New tests added for both async and sync behaviors after refactoring.
src/apify_client/clients/resource_clients/request_queue.py Removed the redundant retry logic; updated type annotations and docstrings accordingly.

@Pijukatel Pijukatel marked this pull request as ready for review April 25, 2025 07:19
@Pijukatel Pijukatel requested review from janbuchar and vdusek April 25, 2025 07:19
Copy link
Contributor

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Breaking change? Are we planning to release client v2? 🙂
We'll probably need to make this backward compatible for now, with a follow-up issue to clean things up for v2.0.

@Pijukatel
Copy link
Contributor Author

Breaking change? Are we planning to release client v2? 🙂 We'll probably need to make this backward compatible for now, with a follow-up issue to clean things up for v2.0.

Good point. Let me just raise deprecation warning when someone uses no longer needed arguments and we can remove them in the far future in v2

@Pijukatel Pijukatel changed the title refactor!: Remove redundant retries from batch_add_requests refactor: Remove redundant retries from batch_add_requests Apr 25, 2025
@Pijukatel Pijukatel requested a review from vdusek April 25, 2025 12:58
Copy link
Contributor

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

thanks

@Pijukatel Pijukatel merged commit 636dc52 into master May 1, 2025
32 checks passed
@Pijukatel Pijukatel deleted the remove-redundant-retries-from-batch-add-requests branch May 1, 2025 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality improvement or decrease of technical debt. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor retries in RequestQueueClient(Async).batch_add_requests
2 participants