-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: server health call implementation #81
Conversation
src/chain/mod.rs
Outdated
connected_rpcs_guard.push(RpcInfo { | ||
rpc_url: endpoint.clone(), | ||
chain_name: c.name.clone(), | ||
status: Health::Critical, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats initial state, all rpcs initialized as Critical until proven differently
src/chain/tracker.rs
Outdated
if let Some(rpc_info) = connected_rpcs_guard.iter_mut().find(|rpc_info| { | ||
rpc_info.rpc_url == *endpoint && rpc_info.chain_name == chain.name | ||
}) { | ||
rpc_info.status = Health::Degraded; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we try to connect we set rpc health degraded
src/chain/tracker.rs
Outdated
if let Some(rpc_info) = connected_rpcs_guard.iter_mut().find(|rpc_info| { | ||
rpc_info.rpc_url == *endpoint && rpc_info.chain_name == chain.name | ||
}) { | ||
rpc_info.status = Health::Ok; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok if successfully connected
src/state.rs
Outdated
@@ -164,6 +173,16 @@ impl State { | |||
Ok(Self { tx }) | |||
} | |||
|
|||
fn overall_health(connected_rpcs: Vec<RpcInfo>) -> Health { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all RPCs are OK, ServerHealth is OK,
if not all OK but some, ServerHealth is Degraded
in none are OK, ServerHealth is Critical
src/chain/tracker.rs
Outdated
@@ -51,7 +54,26 @@ pub fn start_chain_watch( | |||
if shutdown || cancellation_token.is_cancelled() { | |||
break; | |||
} | |||
|
|||
{ | |||
let mut connected_rpcs_guard = connected_rpcs.write(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should never use sync locking primitives in async code. This thing might lock an entire async runtime and thus the program if you accidentally put an awaiting point in a code interval wherein the lock is held.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
src/chain/mod.rs
Outdated
@@ -35,7 +37,8 @@ const SHUTDOWN_TIMEOUT: Duration = Duration::from_millis(120000); | |||
/// RPC server handle | |||
#[derive(Clone, Debug)] | |||
pub struct ChainManager { | |||
pub tx: tokio::sync::mpsc::Sender<ChainRequest>, | |||
pub tx: mpsc::Sender<ChainRequest>, | |||
connected_rpcs: Arc<RwLock<Vec<RpcInfo>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connected_rpcs: Arc<RwLock<Vec<RpcInfo>>>, |
@Slesarew decided to use channels instead of locks everywhere. With this approach, all variables should be declared in an initialization function, not in a struct, like there:
Kalatori-backend/src/chain/mod.rs
Lines 56 to 58 in fe135a3
let mut watch_chain = HashMap::new(); | |
let mut currency_map = HashMap::new(); |
Lines 65 to 76 in fe135a3
let chain_manager = chain_manager.await.map_err(|_| Error::Fatal)?; | |
let db_wakeup = db.clone(); | |
let chain_manager_wakeup = chain_manager.clone(); | |
let currencies = HashMap::new(); | |
let mut state = StateData { | |
currencies, | |
recipient, | |
server_info, | |
db, | |
chain_manager, | |
signer, | |
}; |
I'm against this approach, and plan to refactor it in locks later, but for now I suggest to abide by at least one approach for every module instead of mixing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
9e3af4b
to
a8707d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
Resolves #60