Skip to content

Use rust's type system for special integer parameters - #135

Open
chikko80 wants to merge 1 commit into
CallistoLabsNYC:mainfrom
chikko80:refactor-improve-api
Open

Use rust's type system for special integer parameters#135
chikko80 wants to merge 1 commit into
CallistoLabsNYC:mainfrom
chikko80:refactor-improve-api

Conversation

@chikko80

Copy link
Copy Markdown

I’ve recently started using this library and noticed that it doesn’t make good use of Rust’s type system to describe the API. I’ll probably be working on my own fork most of the time, since I plan to introduce more significant changes. However, I was wondering if this prerequisite change might also be of interest for the main API.

If this is not the direction you want to take this project, please feel free to close this MR, or let me know how we could make it compatible.

Please see the MR description below:

Title

Refactor sentinel integers to typed enums; update Admin API for topic creation

Summary

This PR replaces several raw integer “special values” with strongly typed enums and adjusts the Admin API to use a typed NewTopic structure. This improves correctness, readability, and discoverability, and prevents misuse of magic numbers.

Motivation

  • Remove error-prone magic numbers (e.g., -1, -2) scattered across the protocol and builder APIs.
  • Use rusts type system to provide compile-time guarantees and clear semantics for Kafka protocol flags.
  • Make the admin topic creation API explicit and future-proof.

Key Changes

  1. Produce Required Acks
  • Added prelude::RequiredAcks enum: None (0), Leader (1), All (-1).
  • Refactored:
    • protocol::produce::request::ProduceRequest field required_acks: RequiredAcks.
    • producer::ProduceParams and producer::produce signature.
    • producer_builder::ProducerBuilder::required_acks(RequiredAcks).
  • Encoding implemented via ToByte mapping to the correct INT16 values.
  1. List Offsets
  • Added protocol::list_offsets::request::ReplicaId enum: NormalConsumer (-1) | Broker(i32).
  • Added protocol::list_offsets::request::TimestampQuery enum: Latest (-1) | Earliest (-2) | At(i64).
  • Updated request struct fields and .new/.add signatures to use enums.
  • Updated consumer_builder::list_offsets(...) and related tests to use typed enums.
  1. Create Topics
  • Added protocol::create_topics::request::NumPartitions: Default (-1) | Count(i32).
  • Added protocol::create_topics::request::ReplicationFactor: Default (-1) | Factor(i16).
  • Updated protocol::create_topics::request::Topic and .add(...) to accept these enums.
  • Introduced protocol::create_topics::NewTopic (in mod.rs) with fields:
    • topic_name: String
    • num_partitions: request::NumPartitions
    • replication_factor: request::ReplicationFactor
  • Updated Admin helper admin::create_topics signature to accept Vec<NewTopic> and pass through values.
  • Re-exported NewTopic in prelude for convenience.

Affected Files (high-level)

  • src/lib.rs: re-export RequiredAcks, NewTopic and expose protocol modules in prelude.
  • src/producer.rs, src/producer_builder.rs, src/protocol/produce/request.rs.
  • src/protocol/list_offsets/request.rs, src/consumer_builder.rs, src/protocol/list_offsets/mod.rs (tests).
  • src/protocol/create_topics/request.rs, src/protocol/create_topics/mod.rs (NewTopic), src/admin.rs.
  • Tests and benches updated accordingly.

Backward Compatibility / Breaking Changes

  • Breaking:
    • ProducerBuilder::required_acks(i16)ProducerBuilder::required_acks(RequiredAcks).
    • producer::produce(..., required_acks: i16, ...)required_acks: RequiredAcks.
    • protocol::ProduceRequest::new(required_acks: i16, ...)RequiredAcks.
    • protocol::ListOffsetsRequest::new(..., replica_id: i32)ReplicaId.
    • protocol::ListOffsetsRequest::add(..., timestamp: i64)TimestampQuery.
    • protocol::create_topics::request::Topic fields and .add(...) now use enums.
    • admin::create_topics(..., HashMap<&str, i32>)create_topics(..., Vec<NewTopic>).

Migration Guide

  • Produce:
    • Replace .required_acks(1) with .required_acks(RequiredAcks::Leader).
    • Update direct produce(...) calls to pass RequiredAcks.
  • List Offsets:
    • Use ReplicaId::NormalConsumer or ReplicaId::Broker(id).
    • Use TimestampQuery::{Latest, Earliest, At(ms)} instead of raw i64.
  • Create Topics / Admin:
    • Replace HashMap<&str, i32> with Vec<NewTopic>:
      use samsa::prelude::protocol::create_topics::request::{NumPartitions, ReplicationFactor};
      use samsa::prelude::NewTopic;
      let topics = vec![NewTopic {
          topic_name: "my-topic".into(),
          num_partitions: NumPartitions::Count(3),
          replication_factor: ReplicationFactor::Factor(1),
      }];
      create_topics(conn, corr_id, client_id, topics).await?;

Testing

  • Updated unit/integration tests and benches to use enums.
  • Ran cargo check --tests successfully.

Rationale & Alternatives

  • Enums make the protocol intent explicit and prevent misuse.
  • We intentionally mirrored Kafka protocol sentinel values; encoding maps variants to the expected wire values.
  • Alternative considered: keep integers and add helper constants. Rejected due to lower type-safety and ergonomics.

Checklist

  • Compile-time safety for sentinel values
  • Updated docs and examples
  • Tests updated

@dhonig

dhonig commented Sep 23, 2025

Copy link
Copy Markdown
Collaborator

"

  • Remove error-prone magic numbers (e.g., -1, -2) scattered across the protocol and builder APIs.
    
  • Use rusts type system to provide compile-time guarantees and clear semantics for Kafka protocol flags.
    
  • Make the admin topic creation API explicit and future-proof. "
    

These are great goals for the project. I think this is a good direction for the project."
Give us a few days and we'll figure out if it makes sense to merge this now or after the current release we are working on this week. ( which we have been teeing up all summer.)

Comment thread tests/produce_fetch.rs
CORRELATION_ID,
CLIENT_ID,
1,
prelude::RequiredAcks::Leader,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@chikko80 @hgm-king
I think this shows a great improvement in the readability of the code.

@chikko80 nice work!


#[derive(Debug, Clone, Copy)]
pub enum NumPartitions {
Default,

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I thinking about renaming that to BrokerDefault, to make it more clear


#[derive(Debug, Clone, Copy)]
pub enum ReplicationFactor {
Default,

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I thinking about renaming that to BrokerDefault, to make it more clear

@chikko80 chikko80 changed the title Use rusts type system for special integer parameters Use rust's type system for special integer parameters Sep 23, 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.

2 participants