Refactor: Replace dyn Trait with Enums

[haze518]

Addresses part of ##667

This PR refactors the codebase to replace the use of dyn Trait with enums for Sender, Archiver, State, Storage, and ConfigProvider

[coveralls]

Pull Request Test Coverage Report for Build 12976073614

Details

• 215 of 310 (69.35%) changed or added relevant lines in 42 files are covered.
• 29 unchanged lines in 8 files lost coverage.
• Overall coverage increased (+0.4%) to 75.97%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
server/src/tcp/tcp_tls_listener.rs	0	2	0.0%
server/src/channels/commands/maintain_messages.rs	0	3	0.0%
server/src/streaming/systems/system.rs	4	7	57.14%
server/src/binary/sender.rs	14	18	77.78%
server/src/quic/quic_sender.rs	0	4	0.0%
server/src/streaming/storage.rs	97	103	94.17%
server/src/tcp/tcp_tls_sender.rs	0	7	0.0%
sdk/src/tcp/client.rs	16	25	64.0%
server/src/state/mod.rs	7	16	43.75%
server/src/quic/quic_server.rs	21	39	53.85%

Files with Coverage Reduction	New Missed Lines	%
server/src/tcp/tcp_tls_sender.rs	1	0.0%
server/src/quic/quic_sender.rs	1	71.43%
server/src/streaming/storage.rs	1	94.17%
server/src/streaming/persistence/persister.rs	1	47.19%
server/src/archiver/mod.rs	2	11.11%
server/src/channels/commands/clean_personal_access_tokens.rs	2	87.27%
server/src/archiver/s3.rs	8	25.52%
server/src/streaming/segments/storage.rs	13	42.97%

Totals
Change from base Build 12966998312:	0.4%
Covered Lines:	24836
Relevant Lines:	32692

---

💛 - Coveralls

[spetz]

Looks good, thanks! Before ack, please resolve the conflict with the latest version.

I've also realized, that we could get rid of #[async_trait] added to every Storage trait and its FileStorage implementation.

Could you remove the #[async_trait] from all the traits under server/streaming/storage.rs and their corresponding implementations? This will be another step further into optimizing these things :)

[haze518]

After removing async_trait, I encountered the following warning:

warning: use of `async fn` in public traits is discouraged as auto trait bounds cannot be specified
  --> server/src/streaming/storage.rs:83:5
   |
83 |     async fn load(&self) -> Result<SystemInfo, IggyError>;
   |     ^^^^^
   |
   = note: you can suppress this lint if you plan to use the trait only in your own code, or do not care about auto traits like `Send` on the `Future`
   = note: `#[warn(async_fn_in_trait)]` on by default
help: you can alternatively desugar to a normal `fn` that returns `impl Future` and add any desired bounds such as `Send`, but these cannot be relaxed without a breaking API change
   |
83 -     async fn load(&self) -> Result<SystemInfo, IggyError>;
83 +     fn load(&self) -> impl std::future::Future<Output = Result<SystemInfo, IggyError>> + Send;
   |

To resolve this, I modified the trait to use impl Future, like so:

pub trait PartitionStorage: Send {
    fn load(
        &self,
        partition: &mut Partition,
        state: PartitionState,
    ) -> impl Future<Output = Result<(), IggyError>> + Send;
}

However, after discussing this with @spetz, he mentioned that he didn’t face any warnings after removing async_trait. It’s possible that updating Rust to the latest version (rustup update) or running cargo clean to ensure no outdated build artifacts are causing issues might resolve the problem. I’ll try this approach and see if it works without further adjustments

[spetz]

Yes, I do confirm that it compiles just fine on my side.

[numinnex]

After removing async_trait, I encountered the following warning:

warning: use of `async fn` in public traits is discouraged as auto trait bounds cannot be specified
  --> server/src/streaming/storage.rs:83:5
   |
83 |     async fn load(&self) -> Result<SystemInfo, IggyError>;
   |     ^^^^^
   |
   = note: you can suppress this lint if you plan to use the trait only in your own code, or do not care about auto traits like `Send` on the `Future`
   = note: `#[warn(async_fn_in_trait)]` on by default
help: you can alternatively desugar to a normal `fn` that returns `impl Future` and add any desired bounds such as `Send`, but these cannot be relaxed without a breaking API change
   |
83 -     async fn load(&self) -> Result<SystemInfo, IggyError>;
83 +     fn load(&self) -> impl std::future::Future<Output = Result<SystemInfo, IggyError>> + Send;
   |

To resolve this, I modified the trait to use impl Future, like so:

pub trait PartitionStorage: Send {
    fn load(
        &self,
        partition: &mut Partition,
        state: PartitionState,
    ) -> impl Future<Output = Result<(), IggyError>> + Send;
}

However, after discussing this with @spetz, he mentioned that he didn’t face any warnings after removing async_trait. It’s possible that updating Rust to the latest version (rustup update) or running cargo clean to ensure no outdated build artifacts are causing issues might resolve the problem. I’ll try this approach and see if it works without further adjustments

Regardless if it compiles or not, it is better practice to design those apis in a way that you propose with return position impl trait. My other piece of feedback would be to use generic parameters instead of enums in places where dynamic dispatch is used.

For example

#[derive(Debug)]
pub struct SystemStorage {
    pub info: Arc<SystemInfoStorageKind>,
    pub stream: Arc<StreamStorageKind>,
    pub topic: Arc<TopicStorageKind>,
    pub partition: Arc<PartitionStorageKind>,
    pub segment: Arc<SegmentStorageKind>,
    pub persister: Arc<PersisterKind>,
}

Could be replaced with

#[derive(Debug)]
pub struct SystemStorage<I, ST, T, P, S, PS>
where I: SystemInfoStorage,
ST: StreamStorage,
T: TopicStorage,
P : PartitionStorage,
S: SegmentStorage
PS: Persister
  {
    pub info: Arc<I>,
    pub stream: Arc<ST>,
    pub topic: Arc<T>,
    pub partition: Arc<P>,
    pub segment: Arc<S>,
    pub persister: Arc<PS>,
}

We will merge this pr once you get rid of the async_trait, so don't worry about going back and refactoring it to generics.

[haze518]

My other piece of feedback would be to use generic parameters instead of enums in places where dynamic dispatch is used

Yes, I’ve already thought about this while considering the options. It just seems that in this case, we’d have to add generics everywhere they’re used, which might make the code look overly cumbersome

We will merge this pr once you get rid of the async_trait, so don't worry about going back and refactoring it to generics

Yeah, ok :)

[haze518]

Removed async_trait from the server dependencies.

Left the fix with impl Future based on the comment:

Regardless if it compiles or not, it is better practice to design those apis in a way that you propose with return position impl trait

[spetz]

LGTM :D