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

fix(batch-exports): Add InvalidS3EndpointError as a non-retryable error #26895

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

rossgray
Copy link
Contributor

Problem

Currently we retry a batch export if an invalid S3 endpoint was provided.
https://posthog.sentry.io/issues/5143259713/?project=4506225159897088&referrer=issue-stream&statsPeriod=7d&stream_index=0

Changes

Catch this particular error and raise a custom exception that we can treat as non-retryable.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

it doesn't have an impact

How did you test this code?

Added a test

@rossgray rossgray requested a review from tomasfarias December 13, 2024 12:26
@rossgray rossgray changed the title fix(batch-exports): Add InvalidS3EndpointError as a non-retryable error fix(batch-exports): Add InvalidS3EndpointError as a non-retryable error Dec 13, 2024
@rossgray rossgray force-pushed the ross/batch-exports-non-retry-invalid-url branch from 0e94d32 to fa05aae Compare December 13, 2024 12:33
@@ -166,6 +166,13 @@ def __init__(self):
super().__init__("Endpoint URL cannot be empty.")


class InvalidS3EndpointError(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

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

EmptyS3EndpointURLError is kind of redundant with this, maybe we should remove that and just raise InvalidS3EndpointError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good point, will update

@rossgray rossgray force-pushed the ross/batch-exports-non-retry-invalid-url branch from 1699713 to e30991a Compare December 13, 2024 14:25
@rossgray rossgray force-pushed the ross/batch-exports-non-retry-invalid-url branch from e30991a to e7a5e89 Compare December 13, 2024 14:46
@rossgray rossgray merged commit 8c31447 into master Dec 13, 2024
89 checks passed
@rossgray rossgray deleted the ross/batch-exports-non-retry-invalid-url branch December 13, 2024 15:24
Copy link

sentry-io bot commented Dec 13, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ InvalidS3EndpointError: Invalid endpoint: hel1.your-objectstorage.com posthog.temporal.batch_exports.s3_batch_export ... View Issue

Did you find this useful? React with a 👍 or 👎

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