WAL Benchmark#3775
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR SummaryLow Risk Overview The harness drives a shared Operational pieces mirror other DB sims: JSON Reviewed by Cursor Bugbot for commit c015363. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (11.27%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3775 +/- ##
==========================================
- Coverage 60.14% 59.04% -1.10%
==========================================
Files 2303 2218 -85
Lines 192032 182042 -9990
==========================================
- Hits 115491 107484 -8007
+ Misses 66230 65092 -1138
+ Partials 10311 9466 -845
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c015363. Configure here.
| b.totalRecordsWritten = 0 | ||
| b.totalBytesWritten = 0 | ||
| b.startTimestamp = time.Now() | ||
| fmt.Printf("Benchmark resumed.\n") |
There was a problem hiding this comment.
Resume skips console rate baseline
Low Severity
After resume, totalRecordsWritten and startTimestamp reset but lastConsoleUpdateRecordCount does not. recordsSinceLastUpdate stays negative until the pre-suspend write count is exceeded, so record-based console update throttling is wrong until that catch-up.
Reviewed by Cursor Bugbot for commit c015363. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a self-contained WAL benchmark (walsim) comparing the new seiwal WAL against the legacy WAL via a throwaway shim. This is standalone benchmark/tooling code (no production paths, labeled non-app-hash-breaking); it's well-structured and documented, but has a few config/correctness rough edges worth addressing.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
legacyWALShim.Flushis a permanent no-op because the legacy WAL exposes no explicit sync (confirmed:sei-db/wal/wal.gohas only Write/Truncate/Close). This is inherent, but it undermines the benchmark's stated purpose:FlushIntervalRecordsis meaningful for the seiwal backend and meaningless for legacy, andWalsimMetrics.ReportFlush()is still incremented for legacy on every flush interval even though nothing was flushed — producing a misleading flush metric and a non-apples-to-apples comparison. Consider documenting this limitation prominently and/or suppressing the flush metric for the legacy backend.WalsimConfig.Validatedoes not validate the embeddedSeiwal/Legacysub-configs (no call to theirValidate()), so invalid backend-specific settings are only caught (or not) at open time.REVIEW_GUIDELINES.mdandcursor-review.mdwere empty/missing, so no repo-specific guidelines and no Cursor second-opinion pass were available; only the Codex pass produced output (its three findings are incorporated here).- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| if c.RecordSizeBytes < 1 { | ||
| return fmt.Errorf("RecordSizeBytes must be at least 1 (got %d)", c.RecordSizeBytes) | ||
| } | ||
| if c.RandomDataBufferSizeBytes < c.RecordSizeBytes { |
There was a problem hiding this comment.
[suggestion] Validation gap: this only enforces RandomDataBufferSizeBytes >= RecordSizeBytes, and RecordSizeBytes may be as low as 1, so a config with RandomDataBufferSizeBytes in 1–7 passes Validate(). But crand.NewCannedRandom (sei-db/common/rand/canned_random.go) panics when bufferSize < 8. Since LoadConfigFromFile runs Validate() and then NewWalSim calls NewCannedRandom(int(config.RandomDataBufferSizeBytes), ...), such a config crashes the tool with a panic instead of a clean validation error. Add a minimum (>= 8, or better >= RecordSizeBytes and >= 8) check here.
| break | ||
| } | ||
| // Reset console metrics on resume. | ||
| b.totalRecordsWritten = 0 |
There was a problem hiding this comment.
[suggestion] Resetting totalRecordsWritten to 0 on resume has two side effects beyond console reporting: (1) handleNextRecord gates the periodic flush on totalRecordsWritten % FlushIntervalRecords == 0, so a suspend/resume restarts the flush cadence rather than continuing it; (2) lastConsoleUpdateRecordCount is not reset here, so after resume recordsSinceLastUpdate = totalRecordsWritten - lastConsoleUpdateRecordCount goes negative and the records-based console trigger is effectively disabled until the counter climbs back past the stale value. Consider tracking a separate throughput-window counter, and reset lastConsoleUpdateRecordCount alongside these fields.
| return err | ||
| } | ||
| for _, entry := range entries { | ||
| if err := os.RemoveAll(fmt.Sprintf("%s/%s", dir, entry.Name())); err != nil { |
There was a problem hiding this comment.
[nit] Nit: use filepath.Join(dir, entry.Name()) instead of fmt.Sprintf("%s/%s", ...) for path construction.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — approving despite the nits, since all four inline findings are benchmark-only tooling issues (console-output staleness, suspend/timeout interaction, a benign-error escalation, and a config-validation gap) with no production or consensus code impact.
Extended reasoning...
Overview
This PR adds a new, self-contained benchmark tool (sei-db/seiwal/walsim) for comparing the new seiwal WAL against the legacy sei-db/wal implementation under load. It includes the benchmark driver (walsim.go), a walStore abstraction with a legacy WAL adapter (wal_store.go), a record generator, config/validation, OTel metrics, sample configs, a shell wrapper, and unit tests for the legacy shim's bounds/prune-coalescing behavior. A one-line .gitignore addition ignores the new bin/ build output.
Security risks
None identified. This is standalone CLI benchmark tooling that only touches local WAL files under an operator-specified data directory; it is never imported by or wired into any production/consensus code path (node startup, block execution, RPC, etc.). There is no user-facing input parsing beyond a local JSON config file and command-line args.
Level of scrutiny
Low-to-moderate. This is throwaway benchmark infrastructure (the PR description and code comments explicitly note the legacy WAL/shim are slated for deletion), not production-critical code, so the bar here is correctness of the tool for its intended purpose rather than production hardening. The four issues found by the bug-hunting system are all real but minor: two affect console reporting and suspend/timeout interaction (cosmetic, with authoritative data still exported via Prometheus/OTel), one is a benign-error escalation reachable only via a specific resume-after-reconfiguration scenario on the legacy backend, and one is a validation gap that only surfaces with a deliberately tiny custom buffer size. None of them affect correctness of the core append/flush/prune benchmark loop under normal operation, and none touch production or consensus paths.
Other factors
The PR includes unit tests for the legacy shim's bounds and prune-coalescing logic, and the overall design (shared walStore interface, zero-copy record generation, OTel metrics) is clean and follows patterns used by other DB benchmarks in the repo (cryptosim, blocksim). No CODEOWNER-restricted paths are touched, and there are no unresolved reviewer comments in the timeline.
| if isSuspended { | ||
| b.suspend() | ||
| } | ||
| case <-timeoutChan: | ||
| fmt.Printf("\nBenchmark timed out after %s.\n", | ||
| utils.FormatDuration(time.Since(b.startTimestamp), 1)) | ||
| b.cancel() | ||
| return | ||
| case record := <-b.generator.recordChan: | ||
| b.maybeThrottle() | ||
| b.handleNextRecord(record) | ||
| } |
There was a problem hiding this comment.
🟡 The MaxRuntimeSeconds timeout branch in WalSim.run() returns without calling generateConsoleReport(true), unlike the ctx.Done() (signal/Close) branch, so a run that ends via the documented timeout mechanism skips the forced final throughput summary and also misses the per-loop generateConsoleReport(false) at the bottom of the loop. This is a minor console-output inconsistency between shutdown paths; a one-line fix (add b.generateConsoleReport(true) before the timeout print) would make it consistent with the other exit path.
Extended reasoning...
The bug: In WalSim.run() (sei-db/seiwal/walsim/walsim.go:166-183), the two "planned" shutdown paths handle the final console report differently:
case <-b.ctx.Done():
b.generateConsoleReport(true)
fmt.Printf("\nBenchmark halted.\n")
return
case isSuspended := <-b.suspendChan:
...
case <-timeoutChan:
fmt.Printf("\nBenchmark timed out after %s.\n", ...)
b.cancel()
returnThe ctx.Done() case (hit on SIGINT or an explicit Close()) forces a final report with generateConsoleReport(true) before printing its halt message. The timeoutChan case (hit when MaxRuntimeSeconds elapses — a documented, first-class way to end a benchmark run per WalsimConfig.MaxRuntimeSeconds) prints only the timeout message and returns immediately. Because it returns rather than falling through to the bottom of the for loop, it also skips the per-iteration b.generateConsoleReport(false) call that would otherwise run at the end of every loop body.
Why it's not caught elsewhere: teardown() (deferred via defer b.teardown()) only flushes/closes the WAL and cleans directories — it never touches console reporting, so there's no fallback report on the way out.
Concrete walk-through: Configure MaxRuntimeSeconds: 60, ConsoleUpdateIntervalSeconds: 1. The loop prints a throughput line roughly once per second via generateConsoleReport(false) while records stream in. At t=60s, timeoutChan fires mid-iteration (the goroutine is blocked in select, not at the bottom of the loop). The last printed line reflects whatever counters existed at the last periodic tick (up to ~1s / ConsoleUpdateIntervalRecords stale), and no forced, freshly-computed final line is ever printed — contrast with hitting Ctrl+C, which forces one via generateConsoleReport(true).
Impact: This is a purely cosmetic/console-output gap in benchmark-only tooling (sei-db/seiwal/walsim), not a production or consensus code path. It does not lose data: generateConsoleReport(false) already prints a near-current throughput line at least every ConsoleUpdateIntervalSeconds (default 1s) / ConsoleUpdateIntervalRecords (default 10,000) throughout the run, and all authoritative metrics are exported continuously via OpenTelemetry/Prometheus (walsim_records_written_total, walsim_bytes_written_total, etc.), independent of this console line. So no benchmark data is actually lost — only the final console line is up to ~1 second staler on the timeout path than on the signal/Close path.
Fix: Add b.generateConsoleReport(true) immediately before (or after) the "Benchmark timed out" print in the timeoutChan case, mirroring the ctx.Done() case.
All three independent verifiers confirmed this exact code-level discrepancy and converged on nit severity for the reasons above (benchmark-only tooling, near-current stats already visible on screen, and OTel/Prometheus carrying the authoritative data) — this reasoning agrees with that assessment.
| func (b *WalSim) suspend() { | ||
| // Flush before suspending so state is durable. | ||
| if err := b.store.Flush(); err != nil { | ||
| fmt.Printf("failed to flush on suspend: %v\n", err) | ||
| } | ||
|
|
||
| fmt.Printf("Benchmark suspended.\n") | ||
| b.metrics.SetMainThreadPhase("suspended") | ||
|
|
||
| for { | ||
| select { | ||
| case <-b.ctx.Done(): | ||
| return | ||
| case suspended := <-b.suspendChan: | ||
| if suspended { | ||
| break | ||
| } | ||
| // Reset console metrics on resume. | ||
| b.totalRecordsWritten = 0 | ||
| b.totalBytesWritten = 0 | ||
| b.startTimestamp = time.Now() | ||
| fmt.Printf("Benchmark resumed.\n") | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MaxRuntimeSeconds cannot terminate the run while the benchmark is suspended: run()'s outer select owns the only <-timeoutChan case, but while suspended control sits inside suspend()'s own for/select (only ctx.Done()/suspendChan cases), so a fired deadline goes unnoticed until Resume() is called, at which point it fires immediately. This is benchmark-only, operator-triggered tooling, so it's minor, but worth adding a <-timeoutChan case (or switching to a ctx-based deadline) to suspend()'s loop so a configured run length is a real wall-clock cap even across a manual pause.
Extended reasoning...
run() builds timeoutChan := time.After(MaxRuntimeSeconds) once, and the only place that ever reads from it is the outer loop's select (walsim.go, run()). When that same outer loop receives isSuspended=true on suspendChan, it calls b.suspend() synchronously (line ~174), and suspend() then runs its own blocking for { select { case <-b.ctx.Done(): ...; case suspended := <-b.suspendChan: ... } } } (lines 250-265). That inner select has no timeoutChan case, and while it's running, the outer loop's select (the one that owns timeoutChan) is not being evaluated at all — Go can't observe a channel it isn't currently selecting on.
Concrete trace: start walsim with EnableSuspension=true and MaxRuntimeSeconds=100. At t=10s the operator presses Enter (stdin toggle in cmd/walsim/main.go), Suspend() sends true on suspendChan, and run() calls b.suspend(). Control is now parked in suspend()'s private loop. At t=100s, time.After's channel becomes readable — but nothing is selecting on it, so nothing happens; the benchmark just keeps sitting suspended. Only when the operator presses Enter again does Resume() send false, suspend() returns, and control goes back to run()'s outer select — where timeoutChan is now immediately ready, so the very next iteration terminates the benchmark instantly, regardless of how little "active" time has actually elapsed since resume (e.g., only the original 10s of activity).
So the practical effect is inconsistent, not simply "extended": if the operator never resumes, MaxRuntimeSeconds is never enforced at all (aside from Ctrl+C via ctx.Done()); if they do resume, the benchmark quits at essentially the next event rather than after the intended amount of additional run time. Neither "pause the deadline" nor "treat it as a strict wall-clock cap across pauses" is actually implemented — the timer keeps running invisibly in the background the whole time it's parked in suspend().
I considered the counter-argument (raised by one reviewer) that suspension is a deliberate, operator-driven pause and that having MaxRuntimeSeconds not fire mid-pause is a defensible design choice — the operator is at the keyboard and can Resume() or Ctrl+C at will, and suspend() does reset startTimestamp/counters on resume, suggesting "suspend pauses the run" is intentional. That's a fair reading for the "doesn't self-terminate while paused" half of the behavior. But it doesn't explain the other half: timeoutChan is not actually paused, it keeps counting real wall-clock time from the original start, so resuming after a long pause causes an unrelated-looking near-instant shutdown that has nothing to do with how much the benchmark has run since resume. That's the part that isn't a reasonable design choice so much as a leftover of timeoutChan and the suspend loop not being connected — the fix is cheap (add a case <-timeoutChan: to suspend()'s select, or swap to a single context.WithDeadline that both loops honor via ctx.Done()).
Given this is throwaway benchmark tooling with no consensus/production path, and the worst outcome is a benchmark run's length not matching MaxRuntimeSeconds when the operator manually pauses it, I'm filing this as a nit rather than a blocking issue — it's worth fixing but shouldn't hold up the PR.
| // underlying TruncateBefore, which is expensive in the legacy WAL. The most recent | ||
| // lowestIndexToKeep is used when the forward fires. | ||
| func (s *legacyWALShim) PruneBefore(lowestIndexToKeep uint64) error { | ||
| s.pruneRequestCount++ | ||
| if s.pruneRequestCount%s.pruneRelaxationFactor != 0 { | ||
| return nil | ||
| } | ||
| if err := s.inner.TruncateBefore(lowestIndexToKeep); err != nil { | ||
| return fmt.Errorf("failed to truncate legacy WAL: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🟡 legacyWALShim.PruneBefore (sei-db/seiwal/walsim/wal_store.go:135-137) treats any non-nil error from the legacy WAL's TruncateBefore as fatal, but sei-db/wal's handleTruncate intentionally returns a benign 'out of range' error when the target index is at or behind the current front (documented at wal.go:387-388) — the WAL keeps running, it just reports it. The shim forwards that benign error unmodified, and WalSim.handleNextRecord (walsim.go) escalates any PruneBefore error into b.cancel(), aborting the whole benchmark run instead of continuing like the seiwal backend does (walImpl.PruneBefore never errors for this case).
Extended reasoning...
The bug: sei-db/wal's handleTruncate deliberately treats an out-of-range truncate index (i.e. TruncateFront(idx) where idx is at or behind the WAL's current first offset) as benign — see the doc comment at sei-db/wal/wal.go:387-388: "'Out of range' truncate errors ... are reported to the caller but are not fatal; the WAL continues operating so callers can treat them as benign." However, handleTruncate still writes a non-nil, wrapped error ("out of range truncate error: ...") onto req.errChan, and sendTruncate/TruncateBefore propagate that same non-nil error to the caller regardless of whether it's the benign out-of-range case or a genuinely fatal one — there is no way for a caller to distinguish the two just by checking err != nil.
legacyWALShim.PruneBefore (sei-db/seiwal/walsim/wal_store.go:135-137) does not make that distinction: it wraps and returns any non-nil error from inner.TruncateBefore(lowestIndexToKeep). That propagates up through WalSim.handleNextRecord (walsim.go), which treats any PruneBefore error as fatal — it prints the error and calls b.cancel(), tearing down the entire benchmark run.
Why this is reachable, not just theoretical: In a single continuous run with a fixed config, lowestToKeep grows monotonically and the WAL's firstIndex advances to match, so the target is always in-range and this path never fires. But legacyWALShim is opened against a persistent on-disk WAL (CleanDataOnStart defaults to false), and WalSim.NewWalSim explicitly resumes from existing bounds (Bounds() -> highest, nextIndex = last + 1). retainedRecords is recomputed from the current config's TargetDiskSizeBytes / RecordSizeBytes each time the process starts, not persisted. If a user resumes a legacy-backend run against an existing data directory after raising TargetDiskSizeBytes (or lowering RecordSizeBytes) between runs, retainedRecords grows. The very first forwarded prune in the new run computes lowestToKeep = highestIndex - retainedRecords + 1, which can land below the WAL's persisted firstIndex from the prior run's truncation (the old, smaller retainedRecords had already pruned further ahead). That is exactly the 'out of range' case the WAL treats as benign — but the shim aborts the benchmark instead of ignoring it.
Concrete walkthrough:
- Run 1:
TargetDiskSizeBytes=1GB,RecordSizeBytes=1MB->retainedRecords=1000. After writing 5000 records, prunes have advanced the WAL'sfirstIndexto4001(keeping the most recent 1000). - Process stops (or is restarted) with the same data dir.
- Run 2: config is changed to
TargetDiskSizeBytes=4GB(e.g. testing a larger working set) ->retainedRecords=4000.NewWalSimresumes:Bounds()reportslast=5000, sonextIndex=5001,highestIndex=5000. - The very first new append pushes
highestIndexto5001. Since5001 >= 4000, a prune fires:lowestToKeep = 5001 - 4000 + 1 = 1002. - But the WAL's
firstIndexis already4001(from run 1's pruning) —1002 < 4001, soTruncateFront(1002)is out-of-range for the underlyingtidwall/wallog. handleTruncatereturns the benign-but-non-nil"out of range truncate error". The shim forwards it as a hard error;WalSim.handleNextRecordcallsb.cancel()and the benchmark halts almost immediately, instead of simply no-op'ing the stale prune target and continuing (as theseiwalbackend does viawalImpl.PruneBefore, which never errors on out-of-range targets).
Why nothing currently catches this: wal_store_test.go's TestLegacyShimCoalescesPruneRequests only exercises monotonically increasing, always-in-range prune indices within a single shim instance — it never re-opens the shim against existing data with a different PruneRelaxationFactor/target, so this resume-with-changed-config path is untested.
Fix: In legacyWALShim.PruneBefore, check whether the error from TruncateBefore is the benign 'out of range' case (e.g. strings.Contains(err.Error(), "out of range")) and return nil in that case, mirroring the WAL's own documented benign-error contract and matching the seiwal backend's no-op behavior for the same situation.
| return fmt.Errorf("RecordSizeBytes must be at least 1 (got %d)", c.RecordSizeBytes) | ||
| } | ||
| if c.RandomDataBufferSizeBytes < c.RecordSizeBytes { | ||
| return fmt.Errorf("RandomDataBufferSizeBytes must be at least RecordSizeBytes (%d) (got %d)", | ||
| c.RecordSizeBytes, c.RandomDataBufferSizeBytes) | ||
| } | ||
| if c.StagedRecordQueueSize < 1 { | ||
| return fmt.Errorf("StagedRecordQueueSize must be at least 1 (got %d)", c.StagedRecordQueueSize) | ||
| } | ||
| if c.TargetDiskSizeBytes > 0 && c.TargetDiskSizeBytes < c.RecordSizeBytes { | ||
| return fmt.Errorf("TargetDiskSizeBytes, when non-zero, must be at least RecordSizeBytes (%d) (got %d)", | ||
| c.RecordSizeBytes, c.TargetDiskSizeBytes) | ||
| } | ||
| if c.PruneRelaxationFactor < 1 { | ||
| return fmt.Errorf("PruneRelaxationFactor must be at least 1 (got %d)", c.PruneRelaxationFactor) |
There was a problem hiding this comment.
🟡 WalsimConfig.Validate() only checks RandomDataBufferSizeBytes >= RecordSizeBytes and RecordSizeBytes >= 1, but never enforces crand.NewCannedRandom's 8-byte minimum buffer size. A config with e.g. RecordSizeBytes=1 and RandomDataBufferSizeBytes=4 passes Validate() cleanly but then panics unrecovered in NewWalSim -> crand.NewCannedRandom ("buffer size must be at least 8 bytes, got 4"), so the clean-config-error path Validate() is meant to provide gets bypassed. Fix: also require RandomDataBufferSizeBytes >= 8 (or max(RecordSizeBytes, 8)) in Validate().
Extended reasoning...
WalsimConfig.Validate() (sei-db/seiwal/walsim/walsim_config.go:154-168) enforces RecordSizeBytes >= 1 and RandomDataBufferSizeBytes >= RecordSizeBytes, but never checks that RandomDataBufferSizeBytes meets the 8-byte floor that crand.NewCannedRandom requires (sei-db/common/rand/canned_random.go:40-42):
if bufferSize < 8 {
panic(fmt.Sprintf("buffer size must be at least 8 bytes, got %d", bufferSize))
}Code path: LoadConfigFromFile decodes the JSON config and calls Validate() (sei-db/common/utils/config.go), returning any error to the caller as a normal error — this is explicitly the "clean config error" path the codebase relies on so operators get an actionable message instead of a crash. But NewWalSim (walsim.go) calls crand.NewCannedRandom(int(config.RandomDataBufferSizeBytes), config.Seed) unconditionally after Validate() has already passed, and there is no recover() anywhere between main() and this call.
Step-by-step proof:
- Operator writes a config JSON with
"RecordSizeBytes": 1, "RandomDataBufferSizeBytes": 4. main()->utils.LoadConfigFromFile->WalsimConfig.Validate()runs:RecordSizeBytes (1) >= 1✓RandomDataBufferSizeBytes (4) >= RecordSizeBytes (1)✓- No other check touches
RandomDataBufferSizeBytes, soValidate()returnsnil.
main()proceeds towalsim.NewWalSim(ctx, config, wsMetrics).NewWalSimcallscrand.NewCannedRandom(int(config.RandomDataBufferSizeBytes), config.Seed)=NewCannedRandom(4, seed).NewCannedRandomhitsbufferSize < 8(4 < 8) and panics with"buffer size must be at least 8 bytes, got 4".- No
recover()exists in the call chain, so the process crashes with a raw stack trace instead of the clean, actionable errorValidate()is supposed to guarantee.
Why existing code doesn't prevent it: the two existing checks only relate RandomDataBufferSizeBytes to RecordSizeBytes, not to the library's own absolute floor. Since the default config uses RandomDataBufferSizeBytes: unit.GB, the gap is invisible unless someone deliberately sets a small custom config (e.g., for a minimal smoke test), which is exactly the kind of edge case Validate() exists to catch cleanly.
Fix: add a check such as if c.RandomDataBufferSizeBytes < 8 { return fmt.Errorf(...) } (or fold it into the existing RandomDataBufferSizeBytes >= RecordSizeBytes check via max(RecordSizeBytes, 8)).
Severity note: this is benchmark-only tooling (sei-db/seiwal/walsim), not a production/consensus path, and triggering it requires deliberately setting an unusually tiny buffer size (the shipped defaults are in the MB/GB range). The panic message is also already clear about the root cause. So while the bug is real and Validate() should be fixed to close this gap, it doesn't block any real workflow — marking as a nit.
| // Flush is a no-op. The legacy WAL exposes no explicit sync, so durability is governed entirely by | ||
| // its FsyncEnabled and batching configuration. | ||
| func (s *legacyWALShim) Flush() error { | ||
| return nil |
There was a problem hiding this comment.
it looks default FlushIntervalRecords: 1, the seiwal backend does a real blocking Flush() per record while legacyWALShim.Flush() is a no-op. so the config compares per-record durability against batched writes, does it mean the throughput numbers aren't directly comparable across backends?
walsim_flush_calls_total still increments for legacy even though the flush did nothing, so that metric is inflated for the legacy backend
|
|
||
| if config.CleanDataOnStart { | ||
| fmt.Printf("CleanDataOnStart is enabled, removing contents of: %s\n", config.DataDir) | ||
| if err := removeContents(config.DataDir); err != nil { |
There was a problem hiding this comment.
removeContents(config.DataDir) runs before openWALStore acquires the seiwal directory lock as no path guardrail.
but it's totally fine if we don't have a scenario that we need to run two walsim instance in parallel
2 participants
Describe your changes and provide context
Implements a benchmark for the new WAL implementation.