Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Nov 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 20, 2025 3:08am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 20, 2025 3:08am
rivet-inspector Ignored Ignored Preview Nov 20, 2025 3:08am
rivet-site Ignored Ignored Preview Nov 20, 2025 3:08am

Copy link
Contributor Author

MasterPtato commented Nov 18, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 18, 2025

Code Review - PR #3476

Summary

This PR correctly changes StreamingMode::WantAll to StreamingMode::Exact for all range queries that specify an explicit limit parameter. This is an appropriate optimization based on FoundationDB's streaming mode semantics.

Positive Findings

Correctness

  • The change is semantically correct according to the FoundationDB documentation in engine/packages/universaldb/src/options.rs:193:
    • Exact: "A row limit must be specified if this mode is used"
    • WantAll: "Client intends to consume the entire range"
  • All modified locations specify explicit limits via limit: Some(...), making Exact the appropriate choice

Performance

  • Using Exact mode when you know the exact limit avoids unnecessary data transfer and batching overhead
  • This is particularly beneficial for the affected use cases:
    • download_instances.rs: Downloading a specific count of instances
    • recover_keys_chunk: Recovery operations with fixed batch sizes
    • list_names.rs: List operations with pagination limits
    • debug.rs: Debug queries checking for existence (limit: Some(1))

Consistency

  • I verified that remaining WantAll usages don't specify limits, confirming this PR caught all the relevant cases
  • The change is applied consistently across 6 files

Code Quality

  • One minor improvement in engine/packages/epoxy/src/workflows/replica/setup.rs:443-449: The struct fields were reordered (mode and limit moved before end and begin), which improves readability by grouping the critical query parameters together

Potential Considerations

Testing

  • Consider verifying that the behavior is unchanged for these operations, particularly:
    • download_instances - ensure instance downloads still work correctly
    • recover_keys_chunk - ensure recovery operations complete successfully
    • The debug queries with limit: Some(1) - these should be fast lookups

While the semantic change is minimal (both modes should return the same results when a limit is specified), the different batching behavior could theoretically expose edge cases.

Documentation

  • The PR title and description are minimal. Consider adding context about why this change improves performance (reducing unnecessary batching overhead when exact limits are known).

Verdict

LGTM - This is a well-targeted performance optimization with no apparent bugs or issues. The change correctly applies StreamingMode::Exact to all range queries with explicit limits, which aligns with FoundationDB best practices.

The only suggestion is to ensure adequate test coverage for the affected code paths, though this appears to be a low-risk refactoring.

@claude
Copy link

claude bot commented Nov 18, 2025

PR Review: Change StreamingMode to Exact for ranges with limits

Summary

This PR correctly updates FoundationDB range queries to use StreamingMode::Exact instead of StreamingMode::WantAll when a specific limit is set. This aligns with FoundationDB best practices.

Code Quality ✅

Correctness: The changes are semantically correct. According to FoundationDB documentation:

  • WantAll: For consuming entire ranges with no specific limit
  • Exact: For queries with a specific row limit that should be delivered in a single batch

All modified queries have explicit limits set, making Exact the appropriate choice.

Affected Files:

  1. epoxy/src/replica/messages/download_instances.rs - Fetching log entries with req.count limit
  2. epoxy/src/workflows/replica/setup.rs - Scanning key instances with count limit (also reordered fields for consistency)
  3. gasoline/src/db/kv/debug.rs - Two debug queries with limit: Some(1)
  4. namespace/src/ops/runner_config/list.rs - Listing configs with input.limit
  5. pegboard/src/ops/actor/list_names.rs - Listing actor names with input.limit
  6. pegboard/src/ops/runner/list_names.rs - Listing runner names with input.limit

Performance Considerations ⚡

Positive Impact: Using Exact mode should provide:

  • More predictable performance for limited queries
  • Better resource utilization (single batch vs. progressive fetching)
  • Reduced overhead since the client explicitly knows it wants exactly N rows

Potential Issues 🔍

Minor Note - Field Reordering: In epoxy/src/workflows/replica/setup.rs:445-450, the fields were reordered:

// Before
begin: begin_key,
end: end_key,
mode: StreamingMode::WantAll,
limit: Some(count as usize),

// After  
mode: StreamingMode::Exact,
limit: Some(count as usize),
end: end_key,
begin: begin_key,

While this works fine (fields use named syntax), it's inconsistent with other files in the PR. Consider keeping the original field order for consistency, though this is purely stylistic.

Missing Coverage 🔎

I noticed a few files still using WantAll without limits (which is correct), but wanted to flag these for awareness:

  • pegboard/src/ops/runner/get.rs:109 - No limit (correct as-is)
  • pegboard/src/ops/actor/hibernating_request/list.rs:42 - No limit (correct as-is)
  • pegboard-serverless/src/lib.rs:79 - No limit (correct as-is)

These are intentionally not changed and should remain WantAll.

Test Coverage 📝

Recommendation: While the changes are straightforward, it would be beneficial to verify that:

  1. Queries still return the expected number of results
  2. Behavior is unchanged when the actual data size is less than the limit
  3. Behavior is unchanged when the actual data size exceeds the limit

You could run the affected package tests:

cargo test --package epoxy --package gasoline --package namespace --package pegboard

Security Concerns 🔒

No security issues identified.

Verdict ✅

Approved - This is a good optimization that aligns query patterns with FoundationDB best practices. The changes are minimal, focused, and semantically correct.

Optional: Consider keeping consistent field ordering in setup.rs to match the other files.

Great work on identifying and fixing this pattern across the codebase!

@claude
Copy link

claude bot commented Nov 18, 2025

Code Review

Summary

This PR changes StreamingMode::WantAll to StreamingMode::Exact for all range queries that have explicit limit parameters. This is the correct optimization based on the FoundationDB documentation.

Positives

1. Correct Use of StreamingMode::Exact
According to the StreamingMode enum documentation in engine/packages/universaldb/src/options.rs:192, Exact mode is for clients that have "passed a specific row limit and wants that many rows delivered in a single batch."

All modified queries have explicit limits, making Exact the appropriate mode:

  • download_instances.rs: limit of req.count
  • recover_keys_chunk: limit of count
  • gasoline/debug.rs: limit of 1 (two occurrences)
  • namespace_runner_config_list: limit of input.limit
  • pegboard_actor_list_names: limit of input.limit
  • pegboard_runner_list_names: limit of input.limit

2. Performance Improvement
Using Exact mode eliminates unnecessary overhead from the adaptive batching behavior of WantAll, since these queries know exactly how many rows they need upfront.

3. Consistency
The changes align with existing patterns in the codebase. For example, engine/packages/pegboard/src/workflows/actor/runtime.rs:210 already uses StreamingMode::Exact with limit: Some(1).

Minor Observations

1. Field Reordering in setup.rs
In engine/packages/epoxy/src/workflows/replica/setup.rs:445-450, the fields were reordered (mode/limit/end/begin instead of begin/end/limit/mode). While this doesn't affect functionality, it deviates from the struct definition order per range_option.rs:25-35. This is purely stylistic and not a blocker.

2. Test Coverage
No tests were found specifically for these query functions. While the change is straightforward and correct, consider adding integration tests that verify:

  • Queries return exactly limit rows when more are available
  • Queries return fewer than limit rows when fewer exist
  • Pagination works correctly with the new streaming mode

Security and Correctness

  • No security concerns identified
  • No potential bugs introduced
  • Changes are backward compatible

Verdict

Approved

This is a clean optimization that correctly uses FoundationDB's Exact streaming mode for queries with known limits. The change improves performance without affecting correctness.


Review generated with Claude Code

@claude
Copy link

claude bot commented Nov 19, 2025

Code Review

Summary

This PR correctly changes StreamingMode::WantAll to StreamingMode::Exact for range queries that specify a limit. This is the right change according to FoundationDB's documentation.

Analysis

What's the difference?
According to the StreamingMode enum documentation in engine/packages/universaldb/src/options.rs:

  • WantAll: "Client intends to consume the entire range and would like it all transferred as early as possible."
  • Exact: "The client has passed a specific row limit and wants that many rows delivered in a single batch."

When a query specifies limit: Some(n), using StreamingMode::Exact is the semantically correct choice since we know exactly how many rows we want.

Findings

Correct Changes (6 files):

  • engine/packages/epoxy/src/replica/messages/download_instances.rs - ✓
  • engine/packages/epoxy/src/workflows/replica/setup.rs - ✓ (also reordered fields, which is fine)
  • engine/packages/gasoline/src/db/kv/debug.rs (2 instances) - ✓
  • engine/packages/namespace/src/ops/runner_config/list.rs - ✓
  • engine/packages/pegboard/src/ops/actor/list_names.rs - ✓
  • engine/packages/pegboard/src/ops/runner/list_names.rs - ✓

⚠️ Missed Instance:
There's at least one more instance that should be changed:

engine/packages/gasoline/src/db/kv/mod.rs:

tx.get_ranges_keyvalues(
    universaldb::RangeOption {
        mode: StreamingMode::WantAll,  // <- Should be Exact
        limit: Some(1),
        ..(&pending_signal_subspace).into()
    },
    // ...
)

Recommendations

  1. Update the missed instance in engine/packages/gasoline/src/db/kv/mod.rs for consistency
  2. Consider doing a final sweep to ensure all WantAll + limit combinations are caught

Performance & Correctness

Performance: Using Exact with a limit is more efficient than WantAll because it tells FoundationDB to deliver exactly the requested number of rows in a single batch, avoiding unnecessary data transfer.

Correctness: The change is semantically correct and aligns with FoundationDB best practices.

No Breaking Changes: This is purely an optimization/correctness fix with no API changes.

Overall Assessment

This is a good cleanup PR that improves query efficiency. Once the missed instance is addressed, this will be ready to merge.

Action Items:

  • Update engine/packages/gasoline/src/db/kv/mod.rs to use StreamingMode::Exact where it has limit: Some(1)

@claude
Copy link

claude bot commented Nov 19, 2025

Code Review

Summary

This PR changes StreamingMode::WantAll to StreamingMode::Exact for FoundationDB range queries that have explicit limits. This is a good performance optimization.

Positive Observations

Correct optimization: According to the FoundationDB documentation, when you have a specific row limit, StreamingMode::Exact is the appropriate mode to use. The comment in the options enum states that Exact mode is for when "The client has passed a specific row limit and wants that many rows delivered in a single batch" and "A row limit must be specified if this mode is used."

Using Exact with explicit limits is more efficient than WantAll because:

  • WantAll transfers data early and potentially over-fetches
  • Exact delivers the precise number of rows requested in a single batch

Comprehensive coverage: The PR correctly identifies and updates all instances where mode: StreamingMode::WantAll is used with an explicit limit: Some(...). I verified that the remaining WantAll usages in the codebase do not have explicit limits, which is correct.

Consistent pattern: All 6 changed files follow the same pattern - queries with known limits that should use Exact mode.

Minor Observations

📝 Code organization (epoxy/workflows/replica/setup.rs:446): The field order was changed, moving mode and limit before end and begin. While this doesn't affect functionality, consider maintaining consistent field ordering across similar code blocks for readability. Most other changes kept the original order.

Testing Recommendations

While this is a performance optimization that shouldn't change behavior, consider:

  • Running existing integration tests to ensure queries return the same results
  • Performance testing to validate the expected improvements, especially for the list operations in namespace and pegboard packages

Verdict

Approved - This is a solid optimization that correctly applies FoundationDB best practices. The changes are straightforward and well-targeted.

@MasterPtato MasterPtato force-pushed the 11-17-fix_gas_optimize_loops branch from 2f81c6e to b8b2cef Compare November 20, 2025 03:07
@MasterPtato MasterPtato force-pushed the 11-17-chore_change_streamingmode_to_exact_for_ranges_with_limits branch from fba43a9 to 00a1885 Compare November 20, 2025 03:07
@claude
Copy link

claude bot commented Nov 20, 2025

Code Review

Summary

This PR changes the StreamingMode from WantAll to Exact for range queries that have explicit limits. This is a correct and important optimization based on FoundationDB's documented behavior.

Analysis

What Changed:
The PR updates 6 files where range queries use a specific limit parameter, changing from StreamingMode::WantAll to StreamingMode::Exact.

Why This Is Correct:
According to the FoundationDB documentation in engine/packages/universaldb/src/options.rs:192:

Exact: The client has passed a specific row limit and wants that many rows delivered in a single batch. A row limit must be specified if this mode is used.

WantAll: Client intends to consume the entire range and would like it all transferred as early as possible.

When you have a known, specific limit (like limit: Some(input.limit) or limit: Some(1)), using Exact tells FoundationDB to:

  1. Return exactly that many rows in a single batch
  2. Avoid unnecessary network round trips
  3. Optimize for the known quantity rather than streaming more data

Performance Impact

Positive: This change should improve performance by:

  • Reducing network overhead (single batch vs. multiple)
  • Preventing over-fetching of data
  • More efficient resource usage when limits are known

Code Quality

Consistent Pattern: All changed locations follow the same pattern:

  • They all specify explicit limits
  • They're all using get_ranges_keyvalues
  • The change is applied uniformly

Files Changed

File Limit Type Use Case
epoxy/src/replica/messages/download_instances.rs:42 req.count Downloading specific number of instances
epoxy/src/workflows/replica/setup.rs:446 count Key recovery chunking
gasoline/src/db/kv/debug.rs:521,673 Some(1) Existence checks
namespace/src/ops/runner_config/list.rs:67 input.limit Paginated listing
pegboard/src/ops/actor/list_names.rs:43 input.limit Paginated listing
pegboard/src/ops/runner/list_names.rs:42 input.limit Paginated listing

Potential Considerations

Minor Note - Field Ordering (epoxy/src/workflows/replica/setup.rs)
The field order was reorganized in one file - while this doesn't affect functionality (it's a struct literal), the inconsistent ordering with other changes is slightly unusual. Consider keeping the same field order as the other files for consistency, unless there was a specific reason for the reordering.

Test Coverage

⚠️ Recommendation: While the change is correct, it would be beneficial to:

  1. Run existing integration tests to ensure no regressions
  2. Consider adding a test that validates Exact mode behaves correctly with limits

Security & Safety

✅ No security concerns identified. This is a performance optimization that doesn't change the logical behavior of the queries.

Verdict

LGTM with minor note

This is a good optimization that properly uses FoundationDB's streaming modes. The only minor point is the field reordering in one file, but that's a very minor stylistic issue.


Reviewed following the repository's CLAUDE.md conventions

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.

3 participants