Skip to content

Sync event databases on persist_ping_lifetime_data#3223

Open
badboy wants to merge 1 commit into
mainfrom
push-nozwqlkomyry
Open

Sync event databases on persist_ping_lifetime_data#3223
badboy wants to merge 1 commit into
mainfrom
push-nozwqlkomyry

Conversation

@badboy

@badboy badboy commented Aug 18, 2025

Copy link
Copy Markdown
Member

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

@chutten chutten left a comment

Copy link
Copy Markdown
Contributor

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

badboy commented Aug 20, 2025

Copy link
Copy Markdown
Member Author

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

badboy commented Aug 20, 2025

Copy link
Copy Markdown
Member Author

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.

@chutten chutten left a comment

Copy link
Copy Markdown
Contributor

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