Skip to content

feat: rocks setnx method#223

Closed
heilhead wants to merge 2 commits into
mainfrom
feat/rocks-setnx-method
Closed

feat: rocks setnx method#223
heilhead wants to merge 2 commits into
mainfrom
feat/rocks-setnx-method

Conversation

@heilhead

Copy link
Copy Markdown
Contributor

Description

Note: Do not merge yet as it may break compatibility with existing data. This is scheduled for the 2.0 release.

This adds a proof of concept implementation for the setnx (set if not exists) method to the rocksdb backend.

Caveats:

  • Updates rocksdb dependency to version 9.10.0, which may or may not be compatible with existing data (needs confirmation if we want to release this before the 2.0).
  • Batched writes that we're using for data migration are now performed using transactions, which will conflict with regular writes. This means that the synchronization between data migration and regular writes must now be done outside of the transaction.
  • The new setnx method may not succeed if the data was previously set, because transactions are currently incompatible with pending merge operations. This restriction needs to be made clear to the consumers somehow, that mixing set and setnx on the same key is not allowed.

Also includes some clippy fixes.

How Has This Been Tested?

Existing tests plus some new ones.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a proof-of-concept implementation for the setnx method to the RocksDB backend while also updating various dependencies and refactoring related modules. Key changes include:

  • Adding a new asynchronous setnx API on the string storage type that uses optimistic transactions.
  • Updating string formatting across modules for consistency.
  • Refactoring type aliases and adapting batch and iterator implementations for RocksDB’s transaction API.

Reviewed Changes

Copilot reviewed 14 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/wcn/src/commands/key.rs Updated print formatting for consistent output.
crates/rpc/src/quic/client.rs Changed string formatting in error messages for improved clarity.
crates/rocks/src/lib.rs Introduced type aliases for NativeDb and NativeIterator.
crates/rocks/src/db/types/string.rs Added new setnx method implementation along with tests for its behavior.
crates/rocks/src/db/types/common/iterators.rs Updated iterator types to use NativeIterator alias.
crates/rocks/src/db/types/common.rs Modified iterator return types to support the new NativeIterator.
crates/rocks/src/db/reader.rs Refactored reader functions to utilize the new NativeDb type.
crates/rocks/src/db/migration.rs Revised error message format for export item mismatches.
crates/rocks/src/db/context.rs Reworked Default implementation for DataContext to remove redundant trait bounds.
crates/rocks/src/db/batch.rs Changed WriteBatch configuration from non-transactional to transactional mode.
crates/rocks/src/db.rs Adapted DB accessors and memory usage functions to support NativeDb and transaction support.
crates/node/tests/integration.rs Updated file path formatting in tests.
crates/rocks/Cargo.toml Updated dependency source for rocksdb and refined derive_more configuration.
Cargo.toml Adjusted linter rules for result_large_err and large_enum_variant.
Comments suppressed due to low confidence (2)

crates/rocks/src/db/batch.rs:6

  • Changing the WriteBatchWithTransaction flag from false to true alters the semantics of batched writes. It is recommended to update the relevant documentation/comments to clarify the impact of this change on transaction behavior.
    pub(crate) write_batch: rocksdb::WriteBatchWithTransaction<true>,

crates/rocks/src/db/types/string.rs:49

  • The new setnx method introduces behavior that may conflict with pending merge operations. Consider adding explicit documentation to warn consumers about the limitation of mixing set and setnx on the same key.
    /// Sets value and expiration for a provided key only if the key does not

@heilhead heilhead force-pushed the feat/rocks-setnx-method branch from 887de41 to 8a9a821 Compare June 26, 2025 08:40
created: UnixTimestampMicros,
}

// Manual impl to omit the `T: Default` bounds.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I found a nice macro for that recently https://crates.io/crates/derive-where

@heilhead heilhead closed this Jul 7, 2025
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.

4 participants