Skip to content

Avoid cloning byte arrays in public methods#574

Merged
BusyJay merged 1 commit into
tikv:masterfrom
jkosh44:byte-input
Mar 18, 2026
Merged

Avoid cloning byte arrays in public methods#574
BusyJay merged 1 commit into
tikv:masterfrom
jkosh44:byte-input

Conversation

@jkosh44

@jkosh44 jkosh44 commented Jan 11, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

When the protobuf-codec feature is enabled, bytes::Bytes is used for byte arrays in protobuf objects instead of Vec<u8>. Previously, all of the public methods in RawNode accepted only Vec<u8>. If the caller had an instance of bytes::Bytes, they'd by forced to clone it, pass it to the method, only for the method to convert it back into bytes::Bytes.

This commit updates the public methods of RawNode so that they accept any type that can be converted into bytes::Bytes when protobuf-codec is enabled, and any type that can be converted into Vec<u8> when it's not enabled. This will allow callers to pass in their own bytes::Bytes instead of cloning them.

This change is backwards compatible because Vec<u8> implements both Into<Vec<u8>> and Into<bytes::Bytes>.

Summary by CodeRabbit

Release Notes

  • Refactor
    • Core Raft operation methods (propose, propose_conf_change, and read_index) now accept more flexible input types for context and data parameters, enabling cleaner API usage.

@ti-chi-bot

ti-chi-bot Bot commented Jan 11, 2026

Copy link
Copy Markdown

Welcome @jkosh44! It looks like this is your first PR to tikv/raft-rs 🎉

@jkosh44

jkosh44 commented Jan 11, 2026

Copy link
Copy Markdown
Contributor Author

There isn't an open issue that I could find about this, but it's a simple enough change that it seemed easier to discuss over the implementation rather than an issue.

@jkosh44

jkosh44 commented Mar 14, 2026

Copy link
Copy Markdown
Contributor Author

Just bumping this in case it fell through the cracks. @BusyJay any interest in this change?

@BusyJay

BusyJay commented Mar 16, 2026

Copy link
Copy Markdown
Member

Can you rebase master?

@jkosh44
Avoid cloning byte arrays in public methods
d52bd43 
When the `protobuf-codec` feature is enabled, `bytes::Bytes` is used
for byte arrays in protobuf objects instead of `Vec<u8>`. Previously,
all of the public methods in `RawNode` accepted only `Vec<u8>`. If the
caller had an instance of `bytes::Bytes`, they'd by forced to clone it,
pass it to the method, only for the method to convert it back into
`bytes::Bytes`.

This commit updates the public methods of `RawNode` so that they accept
any type that can be converted into `bytes::Bytes` when `protobuf-codec`
is enabled, and any type that can be converted into `Vec<u8>` when it's
not enabled. This will allow callers to pass in their own `bytes::Bytes`
instead of cloning them.

This change is backwards compatible because `Vec<u8>` implements both
`Into<Vec<u8>>` and `Into<bytes::Bytes>`.

Signed-off-by: Joseph Koshakow <[email protected]>
@coderabbitai

coderabbitai Bot commented Mar 16, 2026 •
edited
Loading

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 13815384-66d6-43e9-a56d-8955a31fc3d3

📥 Commits

Reviewing files that changed from the base of the PR and between b02c962 and d52bd43.

📒 Files selected for processing (2)
  • Cargo.toml
  • src/raw_node.rs
📝 Walkthrough

Walkthrough

Adds the bytes crate as an optional dependency in the protobuf-codec feature and refactors RawNode methods (propose, propose_conf_change, read_index) to accept impl Into<Bytes> instead of Vec<u8>. Introduces a conditional Bytes type alias that resolves to bytes::Bytes when protobuf-codec is enabled, otherwise Vec<u8>.

Changes

Cohort / File(s) Summary
Dependency Configuration
Cargo.toml		 Adds bytes crate as optional dependency included with protobuf-codec feature.
API Refactoring
src/raw_node.rs		 Introduces conditional Bytes type alias and updates three public methods (propose, propose_conf_change, read_index) to accept impl Into<Bytes> for flexible input types instead of Vec<u8>.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • BusyJay

Poem

🐰 Bytes we hop, vectors we chop,
Flexible inputs, conversion won't stop!
Into our methods, data flows free,
Bytes everywhere, as it should be! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: allowing flexible byte input types to avoid cloning in public methods, which is the core purpose of this PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

Tip

You can customize the high-level summary generated by CodeRabbit.

Configure the reviews.high_level_summary_instructions setting to provide custom instructions for generating the high-level summary.

@jkosh44

jkosh44 commented Mar 16, 2026

Copy link
Copy Markdown
Contributor Author

Can you rebase master?

Done.

@BusyJay BusyJay merged commit aafb07c into tikv:master Mar 18, 2026
7 checks passed
@BusyJay

BusyJay commented Mar 18, 2026

Copy link
Copy Markdown
Member

Thanks!

@agourlay agourlay mentioned this pull request Apr 1, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BusyJay BusyJay BusyJay approved these changes

Assignees

No one assigned

Labels

contribution dco-signoff: yes first-time-contributor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jkosh44 @BusyJay