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

DM-43522:Remove too large alert packet write to disk #202

Merged
merged 3 commits into from Mar 27, 2024

Conversation

bsmartradio
Copy link
Contributor

As writing failed alerts to disk is only used for debugging, it should not be on by default and instead a configuration should be set when we want to inspect the alerts.

As writing failed alerts to disk is only used for debugging, it should not be on by default and instead a configuration should be set when we want to inspect the alerts.
Copy link
Member

@kfindeisen kfindeisen 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 some doc suggestions.

@@ -82,6 +82,12 @@ class PackageAlertsConfig(pexConfig.Config):
default=False,
)

doWriteFailedAlerts = pexConfig.Field(
dtype=bool,
doc="Write alerts which fail to send to disk for debugging purposes.",
Copy link
Member

Choose a reason for hiding this comment

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

It's probably worth mentioning that this option is only used if doProduceAlerts, and is independent of doWriteAlerts.

Copy link
Member

Choose a reason for hiding this comment

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

I find the wording a bit hard to parse; maybe:

Suggested change
doc="Write alerts which fail to send to disk for debugging purposes.",
doc="If an alert cannot be sent, write it to disk for debugging purposes.",

@bsmartradio bsmartradio merged commit adfb4da into main Mar 27, 2024
2 checks passed
@bsmartradio bsmartradio deleted the tickets/DM-43522 branch March 27, 2024 00:20
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.

None yet

2 participants