Skip to content

Conversation

@geofflittle
Copy link
Contributor

@geofflittle geofflittle commented Dec 19, 2025

See #605.

This example implementation demonstrates a proposed Store design change.

By lifting the closure level to yield the whole stream instead of individual items, we allow:

  • The primary object being modified/written to be streamed async'ly
  • The secondary object to be async'ly looked-up (e.g., reading pools when validating accounts) rather than pre-loading tables
  • The caller to drive the iteration, allowing for early exits/breaks to save resources
  • Maintaining the implicit, atomic save/commit guarantee of the original design

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

@geofflittle geofflittle requested a review from KtorZ December 19, 2025 07:35
let mut still_active = 0;

// The caller drives the iteration and gets flexibility to short-circuit / break early
// and we keep the implicit atomic buffer write
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be the main argument here: the flexibility and control-flow is given back to the caller. While I agree that it can be nice in certain scenarios; I don't really think we have such scenarios? Updates we have to perform are either on specific entities (single read/write), or entire collections.

Yet, I can totally imagine a store API where we provide both. The suggested approach here has multiple drawbacks though:

  1. It's async. RocksDB isn't capable of doing async operations like this on a single connection. It can do async I/O behind the scene for seek and next operations when explicitly enabled, but not so much for transactional writes. So there's not much benefits in introducing asynchronicity here. It is even annoying because we loose even more determinism and we typically force callers to also be async. At the very least, while the core iterator callback could be async for flexiblity; the outer call to .modify_proposals shouldn't, and we shall use a channel/queue to collect and await results.

  2. I don't think this is abstracting over the right thing. Rather than reworking the existing methods as such, it would make more sense to abstract over the behaviour. So instead of a iter_utxos, iter_proposals, modify_proposals, etc... we could rely on a iter_collection and modify_collection, which can be parameterized by a collection type (then leveraging traits to provide decoders, partial decoders, column prefixes, etc...). This way, we reduce boilerplate significantly, and we make the API way more capable by providing a variety of access with different trade-offs.

Copy link
Contributor

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

cf inline comment.

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.

2 participants