Skip to content
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

Improve error handling for to_apify_request serialization failures #191

Merged
merged 2 commits into from Mar 8, 2024

Conversation

vdusek
Copy link
Contributor

@vdusek vdusek commented Mar 6, 2024

The PR addresses the problem identified in Issue #189, where to_apify_request could fail silently during serialization of the Scrapy request. This PR has introduced enhanced error handling to ensure users are informed if serialization of a request fails, preventing the request from being not enqueued without notification.

@github-actions github-actions bot added this to the 84th sprint - Tooling team milestone Mar 6, 2024
@github-actions github-actions bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Mar 6, 2024
@vdusek vdusek force-pushed the to-apify-request-failing-nicely branch from c6c7f8a to 8b33c0a Compare March 6, 2024 13:35
Copy link
Contributor

@github-actions github-actions bot 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 Tookit has failed!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

@vdusek vdusek added adhoc Ad-hoc unplanned task added during the sprint. debt Code quality improvement or decrease of technical debt. labels Mar 6, 2024
@vdusek vdusek requested a review from fnesveda March 6, 2024 13:40
Copy link
Contributor

@github-actions github-actions bot 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 Tookit has failed!

If issue is not linked to the pull request then estimate the pull request!

@vdusek vdusek force-pushed the to-apify-request-failing-nicely branch 3 times, most recently from 78b4332 to 7ea6fc9 Compare March 7, 2024 12:46
Copy link
Member

@fnesveda fnesveda left a comment

Choose a reason for hiding this comment

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

Looks good, just one note:

Maybe we should log it as errors rather than warnings, since it actually is an issue that the developers should solve. But I agree that it's better to not fail the whole Actor in this case.

@vdusek
Copy link
Contributor Author

vdusek commented Mar 8, 2024

Maybe we should log it as errors rather than warnings, since it actually is an issue that the developers should solve. But I agree that it's better to not fail the whole Actor in this case.

Ok, enqueue_request logs an error message when the conversion fails.

@vdusek vdusek requested a review from fnesveda March 8, 2024 09:22
@vdusek vdusek merged commit 8c5e638 into master Mar 8, 2024
24 checks passed
@vdusek vdusek deleted the to-apify-request-failing-nicely branch March 8, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adhoc Ad-hoc unplanned task added during the sprint. 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.

None yet

2 participants