[r2r] Tendermint multiple rpcs optimization #1568
Conversation
…cs-optimization # Conflicts: # mm2src/coins/tendermint/tendermint_coin.rs
laruh
changed the title
mm2src/coins/lp_coins.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub enum RpcClientEnum { |
There was a problem hiding this comment.
Is it possible to avoid adding such common RPC enum?
In my opinion, RpcClientEnum could be used as a return type of MmCoinEnum::get_rpc_client. As I can see, currently, RpcCommonOps::get_rpc_client is used when the RPC client type is known.
https://github.com/KomodoPlatform/atomicDEX-API/blob/af6a7e7ad7f5ea329e081a63b5df732224b50c7c/mm2src/coins/tendermint/tendermint_coin.rs#L439-L445
In conclusion, I believe that we could refactor this way:
/// Use trait in the case, when we have to send requests to rpc client.
#[async_trait]
pub trait RpcCommonOps {
type RpcClient;
type Error;
/// Returns an alive RPC client or returns an error if no RPC endpoint is currently available.
async fn get_rpc_client(&self) -> Result<Self::RpcClient, Self::Error>;
}Please note that RpcCommonOps::iterate_over_urls is going to be used within RpcCommonOps::get_rpc_client method implementation only, so we can remove it from from the public RpcCommonOps interface.
There was a problem hiding this comment.
Actually thank you for this hint!
I removed enums due to adding types in trait.
So I can just implement this. it's like you suggested below.
impl RpcCommonOps for TendermintCoin {
type RpcClient = HttpClient;
type Error = TendermintCoinRpcError;
async fn get_live_client(&self) -> Result<Self::RpcClient, Self::Error> {please note, that we use get_live_client function like one line only in rpc_client method from trait TendermintCommons.
But I can not remove rpc_client from trait and use just get_live_client instead, bcz we need this function regarding trait CoinCapabilities: TendermintCommons + CoinWithTxHistoryV2 + MmCoin + MarketCoinOps {}.
This explanation is just in case there is a question whether rpc_client should be removed.
| match rpc_client.perform(HealthRequest).await { | ||
| Ok(_) => Ok(RpcClientEnum::TendermintHttpClient(rpc_client.clone())), | ||
| // try HealthRequest one more time | ||
| Err(_) => match rpc_client.perform(HealthRequest).await { | ||
| Ok(_) => Ok(RpcClientEnum::TendermintHttpClient(rpc_client.clone())), | ||
| Err(_) => { | ||
| let new_client = self.iterate_over_urls().await?; | ||
| match new_client { | ||
| RpcClientEnum::TendermintHttpClient(client) => { | ||
| *rpc_client = client.clone(); | ||
| Ok(RpcClientEnum::TendermintHttpClient(client)) | ||
| }, | ||
| _ => Err(RpcCommonError::WrongRpcClient), | ||
| } | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Please consider the following changes:
#[derive(Clone)]
struct TendermintRpcClient(Arc<AsyncMutex<TendermintRpcClientImpl>>);
struct TendermintRpcClientImpl {
rpc_clients: Vec<HttpClient>,
current: usize,
}
#[async_trait]
impl RpcCommonOps for TendermintCoin {
type RpcClient = TendermintRpcClient;
type Error = TendermintCoinRpcError;
async fn get_rpc_client(&self) -> Result<Self::RpcClient, Self::Error> {
let mut rpc /*: AsyncGuard<TendermintRpcClientImpl>*/ = self.rpc_client.lock().await;
match rpc.perform(HealthRequest).await {
// As I suggested above.
Ok(_) => return Ok(self.rpc_client.clone()),
Err(_) => (),
}
// Let's imagine there are 3 RPC clients, and we had an active RPC2:
// | RPC1 | RPC2 | RPC3 |
// |
// Active
//
// Then we should give every RPC client one attempt, but starting with `RPC3`. The order is:
// RPC3, RPC1, RPC2 (one more time).
for ...
}
}There was a problem hiding this comment.
Sorry, could you tell, why do we need Arc here? As I can see we dont need to clone structure and share its instances.
Should we just use this ?
struct TendermintRpcClient {
rpc_urls: Vec<String>,
rpc_client: AsyncMutex<HttpClient>,
current: AtomicUsize,
}There was a problem hiding this comment.
If we don't need to clone, let's avoid using Arc
There was a problem hiding this comment.
Thanks for the code example. I left struct TendermintRpcClient(AsyncMutex<TendermintRpcClientImpl>) and used
type RpcClient = HttpClient;
type Error = TendermintCoinRpcError;UPD also I implemented pushing current url to the end
laruh
changed the title
laruh
changed the title
laruh
changed the title
laruh
changed the title
laruh
changed the title
laruh
changed the title
|
@sergeyboyko0791 I'm waiting for your approval to give it a final review 🙂 |
|
@sergeyboyko0791 Your change request is still pending, could you please recheck this PR? |
optimize handling multiple rpcs #1480 issue
Trait
RpcCommonOpswas implemented forTendermintCointo be able to find first live client and move it to the front.If the live client was the first in vector, than there is a no-op rotation, client will be just returned.