You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "vax-r (via GitHub)" <gi...@apache.org> on 2025/06/17 02:42:44 UTC
[I] Refactor type of `client` field in `IggyClient` [iggy]
vax-r opened a new issue, #1873:
URL: https://github.com/apache/iggy/issues/1873
## Description
Currently, the type of `client` field within `IggyClient` is of type `IggySharedMut<Box<dyn Client>>`, where `dyn Client` is dynamically dispatched.
It can be change to `IggySharedMut<T> where T: Client` so that static dispatch is utilized here.
Execution time performance is expected to get benefit from it, while we need to concern the code bloat after compilation, if certain concrete type of `T` is large, it might make the executable too much larger than before.
## Expected Changes
The usage of `IggyClient` is project-wide, at least very common in SDK.
A thorough research about the relationship between structs within SDK that use `IggyClient` and anything that relates to it is done.
The following part lists out the structs/method that will be change due to this refactor:
* `IggyClient`
* `IggyConsumer`
* `IggyConsumerBuilder`
* `IggyProducer`
* `IggyProducerBuilder`
* `ClientFactory::create_client()`
* `ClientProviderConfig::get_raw_connected_client()`
* `ClientProviderConfig::get_raw_client()`
* `IggyCmdTest::new()`
* `IggyExampleTest`
The parts where use `IggyClient` API shouldn't be changed, the usage of API such e.g. `send()`, should remain the same after the change.
## Expected Result
Switch the `client` field of `IggyClient` entirely from dynamic dispatch to static dispatch. Some performance tests are needed to justify the benefit of this change.
## Note
The change can be project-wide and collide with some on-going work, please let me know if anything is changed against `IggyClient` or other structs/method mentioned above.
I'll be happy to adjust on my side
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "vax-r (via GitHub)" <gi...@apache.org>.
vax-r commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2984885313
Hey @marvin-hansen , thanks for the comment !
I do encountered some questions while tackling this issue.
Since `IggyClient` relates to so many structs, e.g. `IggyClientBuilder` .
They have to be adjust to be static dispatched as well.
In the case for `IggyClientBuilder`, if I want to make it type-generic, it'll look something like
```
#[derive(Debug, Default)]
pub struct IggyClientBuilderV2<T> where T: Client + Default, {
client: Option<T>,
partitioner: Option<Arc<dyn Partitioner>>,
encryptor: Option<Arc<EncryptorKind>>,
}
```
The problem is that now `T` should be known at compile-time, so I don't think putting `with_tcp()`, `with_quic()`, `with_http()` in the block of
```
impl<T> IggyClientBuilderV2<T> where T: CLient + Default, {}
```
is a sane decision.
Maybe put them seperately in
```
impl IggyClientBuilderV2<TcpClient> {
pub fn with_tcp()...
}
impl IggyClientBuilderV2<QuicClient> {
pub fn with_quic()...
}
```
is better?
So is the problem for `from_connection_string()`, since `T` is one of `TcpClient`, `QuicClient`, `HttpClient` , we don't need the `match` block, we can put their separate implementation in the block I mentioned above.
What do you think ?
This is just 1 struct, I think many struct will encounter similar problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "marvin-hansen (via GitHub)" <gi...@apache.org>.
marvin-hansen commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2979240803
Thank you @vax-r
Looks good to me. Fundamentally, the I way I approach this kind of change is the following process:
1) Write a new abstraction i.e. IggyClientStatic while keeping the rest as is.
2) Make sure the code compiles, works as expexted and passes some basic tests.
3) Remove the previous type and then rename IggyClientStatic to IggyClient - Then is when code breaks and test fails
4) Mop up the breaking changes, one crate at a time. If possible, use automated code rewrite i.e. ask ChatGPT or gemini to write you as script to re-write the most common cases. Then, take care of whatever the re-write couldn't handle. By experience, this ranges between 5% to 25% of all cases.
5) Run rustfmt, clippy and double check that all tests pass 2 or 3 times to catch flaky ones.
LMK if any questions come up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "vax-r (via GitHub)" <gi...@apache.org>.
vax-r commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2997223695
How about some usage in test cases, e.g. [/core/tools/src/data-seeder#L66](https://github.com/apache/iggy/blob/3f69440a7e8b62667ccb0820086c91364f263131/core/tools/src/data-seeder/main.rs#L66) and [/core/tools/src/data-seeder#L67](https://github.com/apache/iggy/blob/3f69440a7e8b62667ccb0820086c91364f263131/core/tools/src/data-seeder/main.rs#L67) ?
`get_raw_client()` now becomes `get_raw_client<T>()` , we need to specify the type of client ( either `TcpClient, QuicClient, HttpClient` ) explicitly.
How would you like this part to be refactored ? guess the test should be rewrite ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "marvin-hansen (via GitHub)" <gi...@apache.org>.
marvin-hansen commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2998241364
In a nutshell, I suggest some wrapper like
get_tcp_client()
that dispatches to
get_raw_client<TcpClient>()
If you have any questions, just ask.
Best
On Tue, Jun 24, 2025 at 01:04 I-Hsin Cheng ***@***.***> wrote:
> *vax-r* left a comment (apache/iggy#1873)
> <https://github.com/apache/iggy/issues/1873#issuecomment-2997223695>
>
> How about some usage in test cases, e.g. /core/tools/src/data-seeder#L66
> <https://github.com/apache/iggy/blob/3f69440a7e8b62667ccb0820086c91364f263131/core/tools/src/data-seeder/main.rs#L66>
> and /core/tools/src/data-seeder#L67
> <https://github.com/apache/iggy/blob/3f69440a7e8b62667ccb0820086c91364f263131/core/tools/src/data-seeder/main.rs#L67>
> ?
>
> get_raw_client() now becomes get_raw_client<T>() , we need to specify the
> type of client ( either TcpClient, QuicClient, HttpClient ) explicitly.
>
> How would you like this part to be refactored ? guess the test should be
> rewrite ?
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/iggy/issues/1873#issuecomment-2997223695>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFYR7XFOES774ERE3BBINOD3FAXQBAVCNFSM6AAAAAB7O4L4SCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSOJXGIZDGNRZGU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
spetz closed issue #1873: Refactor type of `client` field in `IggyClient`
URL: https://github.com/apache/iggy/issues/1873
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "vax-r (via GitHub)" <gi...@apache.org>.
vax-r commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2992093268
What about chosen between trait bound and `derive` attribute ?
E.g. refactoring `IggyStreamProducer` to `IggyStreamProducer<T> where T: Client + Default + ... `
Since it has `#[derive(Debug, Default, Clone, Eq, PartialEq)]` , the compiler will complain something like
```
error[E0283]: type annotations needed
--> core/sdk/src/stream_builder/iggy_stream_producer.rs:26:17
|
26 | #[derive(Debug, Default, Clone, Eq, PartialEq)]
| ^ cannot infer type
```
Removing `Clone` and `Default` in `derive` will get through the compiler ( not sure about run-time yet ).
I'm not familiar with the difference between them, I would love to know what do you suggest here ?
Btw, we expect users to put `HttpClient, QuicClient, TcpClient` in `T` when actually using it. Do we have to enforce the same trait bound across all structs/trait that uses this `T` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
spetz commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2993741403
One possible solution would be to have an enum for the internal `client` field (instead of `Box<dyn Client>`), something like e.g. `IggyClientTransport`, with 3 possible variants (`TcpClient`, `QuicClient`, `HttpClient`) and then a bunch of basic pattern matching for the wrapper methods exposed by the `IggyClient`. I'm not sure, though, if that's the best approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio closed issue #1873: Refactor type of `client` field in `IggyClient`
URL: https://github.com/apache/iggy/issues/1873
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "marvin-hansen (via GitHub)" <gi...@apache.org>.
marvin-hansen commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2992976203
@spetz ?
For the second question, adding a trait sho@uld do and get you unstuck.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "marvin-hansen (via GitHub)" <gi...@apache.org>.
marvin-hansen commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2984951621
Sounds about right.
- with_client can go
- IggyClientBuilder<your_type> is good.
I'm a big fan of seperate impl blocks and use them all the time.
Go for it.
On Thu, Jun 19, 2025 at 00:26 I-Hsin Cheng ***@***.***> wrote:
> *vax-r* left a comment (apache/iggy#1873)
> <https://github.com/apache/iggy/issues/1873#issuecomment-2984919495>
>
> Plus, with_client() should no longer be needed right ?
> I've browse through some use cases , when we switched to static
> dispatched, the type of client should be known at compile-time, there's
> no need to use with_client() to create ClientBuilder with certain type,
> just write IggyClientBuilder<your_type> .
>
> That's my understanding.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/iggy/issues/1873#issuecomment-2984919495>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AFYR7XBR6NMWMNOA74AR2LD3EGHLHAVCNFSM6AAAAAB7O4L4SCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSOBUHEYTSNBZGU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "vax-r (via GitHub)" <gi...@apache.org>.
vax-r commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2992215432
> Sounds about right.
>
> - with_client can go
> - IggyClientBuilder<your_type> is good.
>
> I'm a big fan of seperate impl blocks and use them all the time.
>
> Go for it.
> […](#)
After looking into it further, we'll encounter problems when using `from_connection_string()` when putting them in separate blocks.
e.g. for the use cases of `build_iggy_client()` , it's expected that the caller to use generic type. Inside of it we also need to use generic type to call `from_connection_string()` rather than explicitly point it to `IggyClient<TcpClient>, IggyClient<QuicClient>..` in the function body.
I don't think user would want us to implement `build_iggy_client_tcp(), build_iggy_client_quic()...` and so on , not to mention it will continue to split up in the caller.
One way to get around this is to put `from_connection_string()` in a new trait and put it in the trait bound of `T` . Implement an empty block of that trait for `IggyClient<T>` .
But this will make the code look nasty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor type of `client` field in `IggyClient` [iggy]
Posted by "vax-r (via GitHub)" <gi...@apache.org>.
vax-r commented on issue #1873:
URL: https://github.com/apache/iggy/issues/1873#issuecomment-2984919495
Plus, `with_client()` should no longer be needed right ?
I've browse through some use cases , when we switched to static dispatched, the type of `client` should be known at compile-time, there's no need to use `with_client()` to create ClientBuilder with certain type, just write `IggyClientBuilder<your_type>` .
That's my understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org