Skip to content

feat(runner): add pebble sync switch#193

Open
linkdata wants to merge 1 commit into
dnstapir:mainfrom
linkdata:feat/pebble-sync-switch
Open

feat(runner): add pebble sync switch#193
linkdata wants to merge 1 commit into
dnstapir:mainfrom
linkdata:feat/pebble-sync-switch

Conversation

@linkdata
Copy link
Copy Markdown

Summary

  • add --pebble-sync / pebble-sync config support
  • use NoSync for seen-qname writes by default and allow operators to opt into Sync
  • add write option coverage

Tests

  • go test ./pkg/cmd ./pkg/runner ./...

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 30, 2026

Warning

Rate limit exceeded

@linkdata has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 59 minutes and 23 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: faac928b-2db5-4d5f-97fb-98f660246cbf

📥 Commits

Reviewing files that changed from the base of the PR and between b615285 and 31015a0.

📒 Files selected for processing (3)
  • pkg/cmd/run.go
  • pkg/runner/run_minimiser_test.go
  • pkg/runner/runner.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 59 minutes and 23 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@linkdata
Copy link
Copy Markdown
Author

Additional context from comment files

These notes were split from the local markdown comment files and attached here because they describe this PR's change.

Change 1: drop pebble.Sync on the seen-qname write

File: pkg/runner/runner.go, the qnameSeen path (~line 1859).

Before:

if err := pdb.Set([]byte(qname), []byte{}, pebble.Sync); err != nil {

After:

if err := pdb.Set([]byte(qname), []byte{}, pebble.NoSync); err != nil {

Why this was a bottleneck

Pebble's WriteOptions.Sync = true means the WAL is fsynced before the
call returns
. There is no automatic batching, no time-based or
count-based grouping; every Sync write is its own fsync syscall (group
commit shares fsyncs across concurrent goroutines, but our minimiser
workers issue their writes sequentially per worker, so coalescing is
limited).

At a steady 200 K qps with ~10 % novel domains, that is 20 000 fsyncs
per second
to a single Pebble WAL. Even on fast NVMe (≈ 50 µs per
fsync) that is ~ 1 second of fsync wall-clock time per second — i.e.
total saturation of the WAL writer. Worse, while the pipeline is
fsync-bound, the input channel fills, the framestream socket buffer
fills, and our load-generator stalls in pollDesc.waitWrite. This
behaviour shows up in CPU profiles as low runMinimiser utilization
without any single hot function — the cores are waiting.

What we changed

pebble.NoSync — the write is committed to the memtable and appended to
the WAL OS buffer, but does not block on fsync. The WAL is still
flushed periodically by Pebble itself (see Options.WALBytesPerSync,
default 0 = none, and the implicit flush on memtable rotation), and on
graceful shutdown.

Tradeoff

The seen-qname store is a deduplication hint, not a system of record.
Its only purpose is to suppress duplicate new_qname MQTT publications
across restarts. With this change:

  • on a clean shutdown nothing is lost (Pebble flushes its WAL on
    db.Close);
  • on a hard crash you lose at most a few seconds of "we have already
    seen this qname" state;
  • the consequence is that, on next startup, EDM re-publishes those few
    seconds of qnames as new_qname events to MQTT — which is bounded
    (one event per affected name) and self-correcting (subsequent
    observations land in the LRU/Pebble store and dedupe normally).

There is no correctness consequence for downstream parquet output,
histograms, or cardinality counts — those don't read the seen-qname
store. The only observable effect is a small blip of new_qname MQTT
events after a hard crash.

Expected payoff

Removes the per-write fsync entirely. In our load tests this lifts the
MQTT-on ceiling from ~110 K to ~150 K+ qps and makes the system robust
under bursty traffic (no more multi-millisecond input-channel stalls
tied to fsync latency).

If you want to keep durability semantics

A more conservative alternative — not taken in this fork, but suggested
upstream — is to keep pebble.Sync and instead pass these options to
pebble.Open:

&pebble.Options{
    WALMinSyncInterval: func() time.Duration { return 500 * time.Microsecond },
    WALBytesPerSync:    1 << 20,
}

WALMinSyncInterval introduces a 500 µs coalescing window so that
concurrent novel-qname writes from N minimiser workers share a single
fsync. Pebble's commitPipeline.syncLoop does the actual group commit;
the option just widens its waiting window. Net throughput is similar and
durability is preserved at the cost of ~ 500 µs of added publish latency
per affected qname. Worth considering for an upstream PR.


@linkdata linkdata marked this pull request as ready for review April 30, 2026 12:12
@linkdata linkdata requested a review from a team as a code owner April 30, 2026 12:12
@jschlyter jschlyter added the ai AI was used to write contributed code label Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai AI was used to write contributed code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants