Skip to content

feat(bench): enable high-level API for producer and producing consumer - #1919

Merged
numinnex merged 8 commits into
apache:masterfrom
insistedMarin:issue-1856-benchmark-highlevel-api-producer
Aug 7, 2025
Merged

numinnex merged 8 commits into
apache:masterfrom
insistedMarin:issue-1856-benchmark-highlevel-api-producer

Conversation

@insistedMarin

Copy link
Copy Markdown
Contributor

Add HighLevelBackend implementations for producer and producing consumer benchmarks.

This enables the --high-level-api flag to be used across all benchmark types, completing the support started with the high-level consumer backend.

Closes #1856

@insistedMarin
insistedMarin force-pushed the issue-1856-benchmark-highlevel-api-producer branch 2 times, most recently from 2b7fe92 to 799fc85 Compare June 27, 2025 15:14
@numinnex

numinnex commented Jun 27, 2025 •

Copy link
Copy Markdown
Contributor

I am not sure if the abstraction is put in the right place for this feature.

I think it would be better of if the Consumer/Producer was hidden behind another layer (either enum or a struct with generic type determining whether its low-level or high-level client), and it gets method injected into the benchmark scenarios, rather than implementing different backends for each one. This way the code would be more composable, currently there is a lot of code that both looks ugly (those associated types such as (Producer, Identifier, Identifer) ), as well as quite a bit repetitive.

@insistedMarin

Copy link
Copy Markdown
Contributor Author

I am not sure if the abstraction is put in the right place for this feature.

I think it would be better of if the Consumer/Producer was hidden behind another layer (either enum or a struct with generic type determining whether its low-level or high-level client), and it gets method injected into the benchmark scenarios, rather than implementing different backends for each one. This way the code would be more composable, currently there is a lot of code that both looks ugly (those associated types such as (Producer, Identifier, Identifer) ), as well as quite a bit repetitive.

Thank you very much for the feedback! That makes a lot of sense — I’ll go ahead and refactor the code to introduce a unified client trait and inject it into the benchmark logic, as you suggested. This should help reduce duplication and improve composability. Appreciate the detailed suggestion!

@insistedMarin
insistedMarin force-pushed the issue-1856-benchmark-highlevel-api-producer branch from 799fc85 to e13c522 Compare July 5, 2025 21:07
@insistedMarin

Copy link
Copy Markdown
Contributor Author

Thanks again for the helpful feedback, @Numinex!
I've refactored the code by introducing interface-level abstraction at the ConsumerClient and ProducerClient level, which already helped simplify the benchmark logic and reduce duplication.

I haven’t introduced an enum or generic wrapper yet, as it might add unnecessary complexity at this stage. But if you think it’s still worth doing for better composability, I’m happy to revisit and improve it further.

Let me know what you think!

Comment thread core/bench/src/actors/consumer/benchmark_consumer.rs Outdated
@numinnex

numinnex commented Jul 6, 2025 •

Copy link
Copy Markdown
Contributor

That's definitely step in the right direction.

One thing right out of the bat that I would change is moving the setup method to seperate trait something like BenchmarkSetup rather than sticking it to ProducerClient and ConsumerClient.

This way when implementing ProducingConsumingClient, you can put trait bounds on that trait:

pub trait ProducingConsumingClient + ProducerClient + ConsumerClient + BenchmarkSetup

So you don't drag the unnecessary setup methods together with ProducerClient and ConsumerClient.

@insistedMarin
insistedMarin force-pushed the issue-1856-benchmark-highlevel-api-producer branch from e13c522 to 65034ce Compare July 7, 2025 19:58
@insistedMarin

Copy link
Copy Markdown
Contributor Author

That's definitely step in the right direction.

One thing right out of the bat that I would change is moving the setup method to seperate trait something like BenchmarkSetup rather than sticking it to ProducerClient and ConsumerClient.

This way when implementing ProducingConsumingClient, you can put trait bounds on that trait:

pub trait ProducingConsumingClient + ProducerClient + ConsumerClient + BenchmarkSetup

So you don't drag the unnecessary setup methods together with ProducerClient and ConsumerClient.

Thanks for the suggestion! I've split setup and api_label into separate traits as proposed, and cleaned up the client traits accordingly. Let me know if anything else needs adjustment.

Comment thread core/bench/src/actors/consumer/high_level_client.rs Outdated
Comment thread core/bench/src/actors/mod.rs Outdated
Comment thread core/bench/src/actors/consumer/low_level_client.rs Outdated
Comment thread core/bench/src/actors/consumer/benchmark_consumer.rs Outdated
@numinnex

Copy link
Copy Markdown
Contributor

Btw pull the latest changes @insistedMarin I've merged latest master changes to your branch.

@insistedMarin
insistedMarin force-pushed the issue-1856-benchmark-highlevel-api-producer branch from cfe7aaa to f8e08f7 Compare July 13, 2025 18:14
@insistedMarin

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @numinnex! 🙌

I've addressed the requested changes:

Replaced Box / ProducerClient with generic parameters and removed #[async_trait] by making the traits fully generic.

Moved initialization logic into the setup method in BenchmarkInit.

Changed api_label from a method to a const in the ApiLabel trait.

Comment thread core/bench/src/benchmarks/common.rs Outdated
@numinnex

Copy link
Copy Markdown
Contributor

Hi @insistedMarin, sorry for late response, I've checked your changes and it looks pretty good overall, I've left one comment, once you resolve it we can merge this PR.

@numinnex

Copy link
Copy Markdown
Contributor

Hi @insistedMarin, sorry for late response, I've checked your changes and it looks pretty good overall, I've left one comment, once you resolve it we can merge this PR.

BTW I've merged master into your branch, so pull the changes before working on it.

@insistedMarin

Copy link
Copy Markdown
Contributor Author

Hi @numinnex, thanks for the review!
I've removed the commented consumer_group_id as requested.
Let me know if anything else needs to be addressed.

@numinnex

numinnex commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

LGTM, Good job!

@numinnex
numinnex merged commit 1df7254 into apache:master Aug 7, 2025
hageshiame pushed a commit to hageshiame/iggy that referenced this pull request Nov 7, 2025
apache#1919)

Add HighLevelBackend implementations for producer and producing consumer
benchmarks.

This enables the `--high-level-api` flag to be used across all benchmark
types, completing the support started with the high-level consumer
backend.

Closes apache#1856

---------
Co-authored-by: Yang Wong <yang wang>
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.

Add high-level API support for benchmark producer and producing consumer

3 participants