AppDataResponse unnecessarily requires Clone

[schreter]

Currently, AppDataResponse trait requires that the response type implements Clone. However, this is not necessary for openraft, since the response is only moved back to the client, but not otherwise cloned. Therefore, the requirement should be dropped.

diff --git a/openraft/src/lib.rs b/openraft/src/lib.rs
index 103cceac8..3f63c0dd0 100644
--- a/openraft/src/lib.rs
+++ b/openraft/src/lib.rs
@@ -172,13 +172,13 @@ impl<T> AppData for T where T: Clone + Send + Sync + 'static {}
 ///
 /// The trait is automatically implemented for all types which satisfy its supertraits.
 #[cfg(feature = "serde")]
-pub trait AppDataResponse: Clone + Send + Sync + serde::Serialize + serde::de::DeserializeOwned + 'static {}
+pub trait AppDataResponse: Send + Sync + serde::Serialize + serde::de::DeserializeOwned + 'static {}
 
 #[cfg(feature = "serde")]
-impl<T> AppDataResponse for T where T: Clone + Send + Sync + serde::Serialize + serde::de::DeserializeOwned + 'static {}
+impl<T> AppDataResponse for T where T: Send + Sync + serde::Serialize + serde::de::DeserializeOwned + 'static {}
 
 #[cfg(not(feature = "serde"))]
-pub trait AppDataResponse: Clone + Send + Sync + 'static {}
+pub trait AppDataResponse: Send + Sync + 'static {}
 
 #[cfg(not(feature = "serde"))]
-impl<T> AppDataResponse for T where T: Clone + Send + Sync + 'static {}
+impl<T> AppDataResponse for T where T: Send + Sync + 'static {}

Opinions?

(note: in our project, this required us to do unnecessary wrapping/memory allocations and unsafe tricks, which is all not necessary when the Clone requirement is dropped)

Thanks.

[github-actions]

👋 Thanks for opening this issue!

Get help or engage by:

• /help : to print help messages.
• /assignme : to assign this issue to you.

[drmingdrmer]

You are correct; some of the trait bounds have not been given enough consideration.

I will fix this and address similar issues to ensure that all trait bounds are appropriately defined. Please inform me if I missed any. :)

[schreter]

Thanks!

I will fix this and address similar issues to ensure that all trait bounds are appropriately defined. Please inform me if I missed any. :)

I tried to get rid of Clone requirement for AppData as well, but that's obviously impossible. The data is in fact consumed by two consumers - one in apply_to_state_machine where it's passed 1:1 and doesn't require Clone and one for replication. These two have different qualities - the first one needs to produce a reply, the second one doesn't.

An ideal solution would be to model this differently, by having AppDataWriter w/o Clone bound, which is sent by the application and/or read from the log/network and which can then be converted (also multiple times) to a cloneable AppData for replication purposes (for sending over the network). But, that's for now out of scope and I'm not sure if it's a general pattern. Also, it would make modelling the Entry harder. Therefore, I've solved it using some unsafe operations by storing an extra member behind an Arc and moving it out unsafely for apply_to_state_machine.

[drmingdrmer]

Yes it's a bit difficult for now.
An Entry does not necessarily require Clone functionality, but rather it should be capable of transforming some kind of Entry stored in RaftStorage into a transportable one.