Skip to content

Conversation

@tac0turtle
Copy link
Contributor

@tac0turtle tac0turtle commented Jan 2, 2026

Overview

Tune badgerdb for our write heavy usecase, this is a small change i dont believe we need to go beyond this

closes #2605

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 7, 2026, 9:36 AM

@claude
Copy link
Contributor

claude bot commented Jan 2, 2026

Claude finished @tac0turtle's task in 2m 3s —— View job


Code Review Complete

I've completed a comprehensive review of this PR. The BadgerDB tuning changes are well-reasoned for write-heavy workloads, and the benchmark tool is a valuable addition for validation. Below are my findings organized by category.


✅ Strengths

  1. Well-documented rationale: Each BadgerDB option includes clear inline comments explaining the "why"
  2. Conservative tuning: The changes are modest and focused on the specific write pattern
  3. Validation tool: The db-bench tool enables empirical comparison
  4. Deterministic benchmarks: Uses seeded RNG for reproducible results
  5. Clean integration: Options are properly wired through existing store constructors

🔍 Code Quality & Architecture

pkg/store/badger_options.go

Good practices:

  • Clear separation of concerns with dedicated options function
  • CPU-aware compactor scaling with reasonable bounds (4-8)
  • Well-commented configuration choices

Minor suggestions:

  • Consider exposing compactorCount() as a variable or config option for testing different values
  • The function modifies badger4.DefaultOptions - ensure this doesn't cause issues if DefaultOptions is a shared global

tools/db-bench/main.go

Architecture concerns:

  1. Line 67: Ignored error on os.RemoveAll could cause issues:

    if cfg.reset {
        _ = os.RemoveAll(profileDir)  // Error silently ignored
    }

    If removal fails (permissions, locked files), the benchmark runs with stale data, yielding misleading results.

    Fix this →

  2. Repetitive cleanup pattern: The runProfile function has 7 instances of manual kv.Close() calls on error paths (lines 157, 164, 169, 178, 184, 188-190). This is error-prone and violates DRY.

    Better approach: Use defer kv.Close() immediately after successful open at line 136, then remove all manual Close() calls.

    Fix this →

  3. Line 118: rand.NewSource(1) without comment. Consider documenting why seed=1 was chosen:

    rng := rand.New(rand.NewSource(1)) // Fixed seed for reproducible benchmarks

🛡️ Error Handling & Resilience

Issues identified:

  1. pkg/store/kv.go:21: No error context wrapping. If badger4.NewDatastore fails, the caller gets a raw error without knowing which path failed:

    return badger4.NewDatastore(path, BadgerOptions())

    Recommendation: Wrap errors with context:

    db, err := badger4.NewDatastore(path, BadgerOptions())
    if err != nil {
        return nil, fmt.Errorf("failed to create badger datastore at %s: %w", path, err)
    }
    return db, nil

    Fix this →

  2. tools/db-bench/main.go:183-186: Sync errors close the DB but don't return the error properly:

    if err := kv.Sync(ctx, ds.NewKey("/")); err != nil {
        _ = kv.Close()
        return result{}, fmt.Errorf("sync failed: %w", err)
    }

    The Close() error is ignored here. Consider using multierror or logging the Close() failure.


🔒 Security

No critical security issues identified, but observations:

  1. Temporary directory creation (tools/db-bench/main.go:268): Uses os.MkdirTemp with evnode-db-bench-* pattern. This is secure (0700 permissions by default).
  2. File permissions: Explicitly sets 0755 for DB directories (line 69), which is reasonable for benchmark data.
  3. No user input sanitization needed: All inputs are flags parsed by the stdlib flag package with type validation.

⚡ Performance & Resource Efficiency

BadgerDB Tuning Analysis

The tuning choices align well with append-heavy workloads:

  1. WithDetectConflicts(false): ✅ Good for single-writer scenarios. Confirm ev-node doesn't have concurrent writers to the same keys.

  2. WithNumLevelZeroTables(10) (default: 5): ✅ Allows more L0 tables before compaction, smoothing bursty writes.

  3. WithNumLevelZeroTablesStall(20) (default: 15): ✅ Prevents write stalls under heavy load.

  4. WithNumCompactors(4-8) (default: 4): ✅ Scales with CPU, but caps at 8 to avoid compaction thrash. This is conservative and sensible.

Questions for consideration:

  • Have you measured the impact on read latency? More L0 tables can increase read amplification.
  • What's the expected compaction behavior under sustained load? The benchmark is burst-oriented.
  • Consider adding a -cpus flag to db-bench to test compactor scaling behavior.

Benchmark Tool Performance

  1. Batch size default (1000): Reasonable for testing, but consider if real workload uses different batch sizes.
  2. Deterministic RNG: Good for reproducibility, but the data is compressible. Badger uses compression by default - is this representative of real data?
  3. Memory usage: The benchmark doesn't track memory usage or GC pressure. Consider adding runtime.MemStats reporting.

🧪 Testing & Reliability

Major concern: 7.10% patch coverage (170 lines missing)

  1. No tests for BadgerOptions(): Should verify:

    • Conflict detection is disabled
    • L0 table counts are correct
    • Compactor count respects bounds (4 ≤ count ≤ 8)
    • Returns non-nil options
  2. No tests for benchmark tool: While it's a CLI tool, core logic could be unit tested:

    • dirSize() function
    • humanBytes() formatting
    • selectProfiles() logic
    • compactorCount() boundary conditions
  3. Integration test gap: No test verifies the tuned options actually work with the store initialization path.

Recommendations:

// Example test structure
func TestBadgerOptions(t *testing.T) {
    opts := BadgerOptions()
    require.NotNil(t, opts)
    
    // Verify key settings
    assert.False(t, opts.Options.DetectConflicts)
    assert.Equal(t, 10, opts.Options.NumLevelZeroTables)
    assert.Equal(t, 20, opts.Options.NumLevelZeroTablesStall)
    
    compactors := opts.Options.NumCompactors
    assert.GreaterOrEqual(t, compactors, 4)
    assert.LessOrEqual(t, compactors, 8)
}

func TestCompactorCount(t *testing.T) {
    tests := []struct {
        name     string
        cpuCount int
        want     int
    }{
        {"low cpu", 2, 4},
        {"mid range", 6, 6},
        {"high cpu", 16, 8},
    }
    // Test with mocked runtime.NumCPU()
}

Fix this →


📚 Documentation & Developer Experience

Good aspects:

  • README updated to mention tuned options and benchmark tool
  • Inline comments explain each option's purpose
  • db-bench README provides clear usage examples

Improvements needed:

  1. Missing context in pkg/store/README.md: The update mentions tuned options but doesn't explain:

    • What workload patterns benefit from these settings
    • When to use default vs. tuned options
    • Trade-offs (e.g., read latency impact)

    Suggested addition (around line 9):

    Badger options are tuned for the ev-node write pattern (append-heavy with periodic overwrites) via `store.BadgerOptions()`. 
    
    Key tuning decisions:
    - Disabled conflict detection (single-writer assumption)
    - Increased L0 table limits to handle bursty writes
    - Scaled compactors based on CPU count
    
    Trade-off: Higher L0 table counts may increase read amplification. Use `tools/db-bench` to validate performance against Badger defaults for your workload.
  2. tools/db-bench/README.md: Should document:

    • What the benchmark measures vs. doesn't measure (e.g., no read performance, no concurrency)
    • How to interpret results
    • Expected performance ranges
  3. Missing godoc for compactorCount(): Should be documented or marked as internal.


🎯 Summary & Recommendations

Must Fix

  1. Handle os.RemoveAll error in tools/db-bench/main.go:67
  2. Use defer for kv.Close() in runProfile() to prevent resource leaks
  3. ⚠️ Add tests for BadgerOptions() to prevent regression

Should Fix

  1. 🔧 Wrap errors with context in NewDefaultKVStore and NewTestInMemoryKVStore
  2. 🔧 Improve documentation in pkg/store/README.md about tuning trade-offs
  3. 🔧 Add tests for benchmark tool utility functions

Nice to Have

  1. 💡 Consider exposing compactor count as a tunable parameter
  2. 💡 Add memory usage tracking to db-bench
  3. 💡 Document benchmark interpretation guidelines

Verdict

This is a solid, focused PR with well-reasoned tuning decisions. The benchmark tool is a valuable addition for validation. However, the lack of tests is concerning - production database configuration should have test coverage to prevent regressions.

Recommendation: ✅ Approve with requested changes - fix the error handling issues and add basic tests for BadgerOptions().

The tuning approach is conservative and well-documented. Once testing is added, this will be a high-quality contribution.


@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the performance of the BadgerDB key-value store by introducing a set of finely-tuned default options tailored for the ev-node's specific write workload. To support validation and comparison, a new benchmarking utility has also been added, allowing developers to assess the impact of these optimizations.

Highlights

  • BadgerDB Option Tuning: Introduced a new BadgerOptions() function in pkg/store/badger_options.go to provide a set of tuned configurations for BadgerDB, specifically optimized for ev-node's append-heavy write patterns with periodic overwrites.
  • Configuration Details: The tuning includes disabling conflict detection, increasing NumLevelZeroTables to 10 and NumLevelZeroTablesStall to 20 to handle bursty writes, and dynamically scaling NumCompactors based on CPU count (between 4 and 8).
  • Integration of Tuned Options: The NewDefaultKVStore and NewTestInMemoryKVStore functions in pkg/store/kv.go were updated to utilize these new BadgerOptions() by default.
  • New Benchmarking Tool: A new command-line tool, db-bench, has been added under tools/db-bench. This tool allows for local benchmarking of BadgerDB performance, comparing the ev-node tuned options against Badger's default settings.
  • Documentation Updates: The pkg/store/README.md was updated to mention the tuned Badger options and the existence of the db-bench tool for validation. A new tools/db-bench/README.md was also added to document the usage of the benchmarking tool.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces performance tuning for BadgerDB by providing a set of default options tailored for the application's write patterns. It also adds a new benchmark tool, db-bench, to validate these performance improvements. The changes are well-structured and the new options are correctly applied. The benchmark tool is a valuable addition. I have provided a couple of suggestions to improve the implementation of the benchmark tool for better error handling and code clarity.

for _, p := range selected {
profileDir := filepath.Join(baseDir, p.name)
if cfg.reset {
_ = os.RemoveAll(profileDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The error returned by os.RemoveAll is ignored. If removing the directory fails (e.g., due to permissions), the benchmark might run with old data or fail at a later stage, leading to confusing results. It would be better to handle this error, for instance by logging a warning to the user.

Suggested change
_ = os.RemoveAll(profileDir)
if err := os.RemoveAll(profileDir); err != nil {
fmt.Fprintf(os.Stderr, "Warning: failed to remove profile directory %s: %v\n", profileDir, err)
}

@codecov
Copy link

codecov bot commented Jan 2, 2026

Codecov Report

❌ Patch coverage is 7.10383% with 170 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.67%. Comparing base (42228e2) to head (ced8274).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
tools/db-bench/main.go 0.00% 166 Missing ⚠️
pkg/store/badger_options.go 71.42% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2950      +/-   ##
==========================================
- Coverage   59.15%   57.67%   -1.48%     
==========================================
  Files          90       92       +2     
  Lines        8632     8902     +270     
==========================================
+ Hits         5106     5134      +28     
- Misses       2944     3179     +235     
- Partials      582      589       +7     
Flag Coverage Δ
combined 57.67% <7.10%> (-1.48%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tac0turtle tac0turtle requested a review from julienrbrt January 7, 2026 09:36
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.

perf: configure db options

2 participants