Skip to content
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

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

Vovke
Copy link
Collaborator

@Vovke Vovke commented Oct 10, 2024

Resolves #60

src/chain/mod.rs Outdated
connected_rpcs_guard.push(RpcInfo {
rpc_url: endpoint.clone(),
chain_name: c.name.clone(),
status: Health::Critical,
Copy link
Collaborator Author

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

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;
Copy link
Collaborator Author

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

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;
Copy link
Collaborator Author

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 {
Copy link
Collaborator Author

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

@Vovke Vovke requested review from Slesarew and fluiderson October 10, 2024 21:45
Cargo.toml Outdated Show resolved Hide resolved
@@ -51,7 +54,26 @@ pub fn start_chain_watch(
if shutdown || cancellation_token.is_cancelled() {
break;
}

{
let mut connected_rpcs_guard = connected_rpcs.write();
Copy link
Collaborator

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.

Copy link
Collaborator Author

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>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:

let mut watch_chain = HashMap::new();
let mut currency_map = HashMap::new();

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

@Vovke Vovke marked this pull request as draft October 14, 2024 08:50
@Vovke Vovke force-pushed the api-specs branch 2 times, most recently from 9e3af4b to a8707d4 Compare October 15, 2024 16:56
@Vovke Vovke marked this pull request as ready for review October 15, 2024 16:58
Copy link
Collaborator

@fluiderson fluiderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Slesarew Slesarew merged commit d5920af into Alzymologist:main Oct 21, 2024
4 of 6 checks passed
@Vovke Vovke deleted the api-specs branch November 11, 2024 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Health call according to specs
3 participants