minjibir commented on PR #489: URL: https://github.com/apache/pekko-persistence-r2dbc/pull/489#issuecomment-5788548052
Thanks for the detailed round. All points are addressed: **1. db_timestamp sampled at enqueue** — Rows are now stamped once per flush, immediately before `writeEvents`, and re-stamped on bisection retries. The published timestamp is the value `writeEvents` returns (`events.head.dbTimestamp`), so pub-sub offsets match the stored rows. `R2dbcBatchJournalPublishTimestampSpec` asserts the shared flush timestamp and the per-envelope offsets. `journal.md` documents the bound: connection acquisition plus one transaction, so `query.behind-current-time` must stay above that lag. **2. Queue-full rejection trips the circuit breaker** — Queue-full is now a per-message rejection in a successful `Future` (the `AsyncWriteJournal` rejection path), not a failed `Future`, so the circuit breaker never counts it. The actor-level consequence is pekko-persistence's standard handling for any rejected write: a classic persistent actor logs it in `onPersistRejected` by default and continues without storing the event; a typed persistent actor restarts with an `EventRejectedException`. `R2dbcBatchJournalQueueLimitSpec` pins the boundary via a debug log and asserts the exact accept/reject set, and a second test pushes 12 rejections (more than `max-failures`) and asserts a subsequent write succeeds. The `max-queue-size` comment in `reference.conf` and `journal.md` now describe the per-message rejection instead of the old whole-Future failure. **3. Bisection retry inside the single flush slot** — The halves now retry in parallel (`write(left).zipWith(write(right))`), so retry latency is about one transaction per level instead of two serial ones. The flush slot is still held: `FlushDone` only fires after both halves finish, keeping the single-in-flight invariant. `journal.md` Failures now says the other persistent actors wait while the retries run. Per-request savepoints would need `JournalDao` changes; I left that for a follow-up. Concurrency narrows one guarantee, and the class scaladoc now states it: each half stamps its own timestamp, and a single split preserves order, but if the half holding the earlier sequence numbers is retried after its sibling committed, its re-stamped rows can invert the `db_timestamp` order of two same-persistence-id writes. Replay still orders by sequence number and recovery is unaffected. Only timestamp-ordered read sides see it, and only with an integrity violation plus two in-flight writes for the same pid in one batch. **4. Flush timer dropped mid-flush** — The benchmark compares the two flush policies, not the exact mid-flush-arrival example: it does not isolate a single request whose timer fires and is dropped during a flush. That case costs a mid-flush arrival one extra batch window under the re-arm (flush remainder plus max-batch-time), which is inside the documented bound of one flush plus one window. At the policy level the benchmark is still decisive. At ~166 w/s the flush-on-completion variant makes 225 flushes for 1000 writes, including 54 single-record flushes (24%); the re-arm makes 180 flushes and no single-record flush at all (smallest is 2 records). Wall time is identical (3,085 / 3,071 / 3,023 ms for 1000 writes at ~330 w/s), and below that rate all variants are injection-bound, so the extra flushes buy nothing. The runs were against a live database container with `max-batch-time = 2ms` (effective one 10ms scheduler tick); I can share the full flush-count tables on request. The real cost of the re-arm is the fixed window on quiet writes: a sequential actor pays about 30.5 ms per write against about 2.5 ms for the default journal. A follow-up will flush immediately when the queue is empty and no flush is in flight, keeping the timer accumulation after a flush completes. That removes the idle penalty without the single-record regression. The "sustained load" sentence in `journal.md` was imprecise for loads below `max-batch-size` per flush; it now states the exact condition for back-to-back flushes. **5. max-batch-time below the scheduler tick** — The default `max-batch-time` is now 10ms, matching the scheduler tick. `reference.conf` and `journal.md` document that sub-tick values round up to whole ticks, and that lowering `pekko.scheduler.tick-duration` changes the timer resolution for the entire actor system. The previous 2ms was already effective 10ms, so the perf spec results are unchanged. **6. Dialect require justified with RETURNING** — The Scaladoc and `journal.md` now say the plugin is only supported and tested for the Postgres and Yugabyte dialects. I kept the `require` as a guardrail. Dropping it so the MySQL CI job runs the specs is possible as a follow-up if you want it. **7. JournalDao.writeEvents mixed-pid contract** — The `writeEvents` scaladoc now documents the mixed-pid batch use and the `db-timestamp-monotonic-increasing` requirement, and the two "Wrote N events" log lines use `totalEvents` instead of the hard-coded 1, which was wrong for the default journal as well. **8. QueueLimitSpec off-by-one gap** — The spec now waits for the flush to dequeue the first write (via a debug log captured with `LoggingTestKit`) before sending the boundary writes, and asserts the exact accept/reject set, so an off-by-one in the queue-full check fails the test. **9. License headers on copied files** — The Sep 11 instruction applied to new code. These four files copy code from Akka-derived files, so the repo's licensing rule for copied code applies as well: they keep the standard header you pointed to, with the derived-from-Akka sentence and the Lightbend copyright appended for the copied portions. Provenance is noted in the PR body. **10. Duplication with R2dbcJournal** — I kept the duplication deliberately. Issue #471's accepted design is one self-contained class that reuses existing internals without restructuring them, so upstream syncs stay conflict-free. Extracting shared helpers touches `R2dbcJournal` and the replay and highest-seq-nr paths that #487 recently changed. I can do the extraction as a follow-up PR once this lands, if you agree. **Minor** — The enqueue path now calls `doFlush()` directly instead of sending a `Flush` message. The publish method is now synchronous and takes the resolved timestamp directly, eliminating `Future` allocations entirely. The `stopping` flag and the shutdown guards are removed as suggested; the remaining self-sends after stop are dead letters, which Pekko does not log during shutdown by default. The `use-app-timestamp` comment in `reference.conf` now states the MySQL and batched-journal requirement; the yugabyte issue link is kept. **Checked and not an issue** — Thanks for checking those. The pub-sub publish is best-effort as you say; the database query is the backstop for the offsets. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
