Skip to content

Conversation

badboy
Copy link
Member

@badboy badboy commented Aug 18, 2025

Instead of #3220

I'd like to defer this until after the release.

@badboy badboy requested a review from a team as a code owner August 18, 2025 10:20
@badboy badboy requested review from chutten and removed request for a team August 18, 2025 10:20
Copy link
Contributor

@chutten chutten left a comment

Choose a reason for hiding this comment

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

Needs CHANGELOG.md

API design: Yeah, okay, it kinda tracks. Events are ping-lifetime and this does persist them.

Tests: I guess we need to record events and then forceably crash the app to prove that persist_ping_lifetime_data is necessary to persist events to disk now?

Instrumentation: Do we want to monitor how often sync_all errors out? Seems likely. Though if it's recorded in a ping-lifetime metric we might not ever receive it.

Logging: Is it an error? There's nothing anyone can do about it. Seems like at most a warn. Do we want to spend some words on why this might matter (e.g. "Recently-recorded events for ping {file} have not been persisted to disk and may be lost")?

@badboy
Copy link
Member Author

badboy commented Aug 20, 2025

Logging: Is it an error? There's nothing anyone can do about it. Seems like at most a warn. Do we want to spend some words on why this might matter (e.g. "Recently-recorded events for ping {file} have not been persisted to disk and may be lost")?

The problem with that wording is: it's not accurate. if fsync fails we don't really know whether data is safely persisted or not. Or which data. Anyway, fun!

@badboy
Copy link
Member Author

badboy commented Aug 20, 2025

Tests: I guess we need to record events and then forceably crash the app to prove that persist_ping_lifetime_data is necessary to persist events to disk now?

It would be much more complicated than that. Even on crashes most likely data is written to disk just fine if the app worked normally before the crash.
Testing that code path would require injecting IO faults.

Copy link
Contributor

@chutten chutten left a comment

Choose a reason for hiding this comment

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

(about testing) Fair 'nuff.

(Still needs CHANGELOG)

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