Add wait-for-sbot to peach-config #131

notplants · 2022-07-12T10:21:42Z

notplants commented

2022-07-12 10:21:42 +00:00

As discussed here, this PR adds a wait-for-sbot function peach-config.

It tries to connect to sbot and call whoami, and if it successfully returns, then it returns a success, and we know the sbot is ready for connections.

If whoami fails, it waits two seconds, and then tries again. I tries a maximum of 10 times, and then it gives up and returns an error.

As discussed [here](https://git.coopcloud.tech/PeachCloud/peach-workspace/issues/127#issuecomment-13603), this PR adds a wait-for-sbot function peach-config. It tries to connect to sbot and call whoami, and if it successfully returns, then it returns a success, and we know the sbot is ready for connections. If whoami fails, it waits two seconds, and then tries again. I tries a maximum of 10 times, and then it gives up and returns an error.

notplants added 2 commits 2022-07-12 10:21:44 +00:00

cb09d6c3e9 Wait for sbot

continuous-integration/drone/pr Build is failing Details

29f5ad0e84 Wait for sbot is working

notplants added 1 commit 2022-07-12 10:29:52 +00:00

continuous-integration/drone/pr Build is passing Details

fc50bb5ee5 Cargo fmt

ahdinosaur commented

2022-07-12 11:13:31 +00:00

First-time contributor

hi @notplants, here from the "P4P Rust Learning" chat. 😺

on how to refactor peach-config/src/wait_for_sbot.rs, here's what i came up with so far:

use std::{thread, time};

use crate::error::PeachConfigError;
use peach_lib::sbot::init_sbot;

static MAX_NUM_ATTEMPTS: u8 = 10;

/// Utility function to wait for a successful whoami call with sbot
/// After each attempt to call whoami it waits 2 seconds,
/// and if after MAX_NUM_ATTEMPTS (10) there is no successful whoami call
/// it returns an Error. Otherwise it returns Ok(sbot_id).
pub async fn wait_for_sbot() -> Result<String, PeachConfigError> {
    let mut num_attempts = 0;
    let mut whoami = None;

    while num_attempts < MAX_NUM_ATTEMPTS {
        let mut sbot = None;

        let sbot_res = init_sbot().await;
        match sbot_res {
            Ok(sbot_instance) => {
                sbot = Some(sbot_instance);
            }
            Err(err) => {
                eprintln!("failed to connect to sbot: {:?}", err);
            }
        }

        if sbot.is_some() {
            let sbot_id_res = sbot.unwrap().whoami().await;
            match sbot_id_res {
                Ok(sbot_id) => {
                    whoami = Some(sbot_id);
                    break;
                }
                Err(err) => {
                    eprintln!("whoami failed: {:?}", err);
                }
            }
        }

        println!("trying again {:?}", num_attempts);
        num_attempts += 1;

        let sleep_duration = time::Duration::from_secs(2);
        thread::sleep(sleep_duration);
    }

    whoami.ok_or(PeachConfigError::WaitForSbotError {
        message: "Failed to find sbot_id after 10 attempts".to_string(),
    })
}

changes i made:

i removed sbot_is_running and instead used a break to exit the loop
- side note on sbot_is_running, shouldn't this have been a bool? i'm not sure if there's a reason for this to be an integer, when it seems like 1 is being used as true and 0 is being used as false.
i moved the nested matches so they are sequential. the second one will still only run if the first succeeds.
i moved max_num_attempts to be a static constant
i changed two_seconds to be named sleep_duration, might be a personal preference but imo the variable name should describe the purpose of what's in the container. so later when you wanna change how many seconds, you don't need to change that it's a sleep_duration.

i'm curious, what bothers you about match?

another approach (an alternative to the while loop) would be to use recusive functions, but i'm still not sure how to do that without match.

if you plan to unwrap both cases (Ok and Err), afaik match is great for this. if you only want to unwrap Ok, then i'd use an if let. mayyybeee there's a way to do this with the Result methods?

anyways, i hope this helped, happy to review pull requests anytime. 💜

hi @notplants, here from the "P4P Rust Learning" chat. 😺 on how to refactor `peach-config/src/wait_for_sbot.rs`, here's what i came up with so far: ```rust use std::{thread, time}; use crate::error::PeachConfigError; use peach_lib::sbot::init_sbot; static MAX_NUM_ATTEMPTS: u8 = 10; /// Utility function to wait for a successful whoami call with sbot /// After each attempt to call whoami it waits 2 seconds, /// and if after MAX_NUM_ATTEMPTS (10) there is no successful whoami call /// it returns an Error. Otherwise it returns Ok(sbot_id). pub async fn wait_for_sbot() -> Result<String, PeachConfigError> { let mut num_attempts = 0; let mut whoami = None; while num_attempts < MAX_NUM_ATTEMPTS { let mut sbot = None; let sbot_res = init_sbot().await; match sbot_res { Ok(sbot_instance) => { sbot = Some(sbot_instance); } Err(err) => { eprintln!("failed to connect to sbot: {:?}", err); } } if sbot.is_some() { let sbot_id_res = sbot.unwrap().whoami().await; match sbot_id_res { Ok(sbot_id) => { whoami = Some(sbot_id); break; } Err(err) => { eprintln!("whoami failed: {:?}", err); } } } println!("trying again {:?}", num_attempts); num_attempts += 1; let sleep_duration = time::Duration::from_secs(2); thread::sleep(sleep_duration); } whoami.ok_or(PeachConfigError::WaitForSbotError { message: "Failed to find sbot_id after 10 attempts".to_string(), }) } ``` changes i made: - i removed `sbot_is_running` and instead used a `break` to exit the loop - side note on `sbot_is_running`, shouldn't this have been a `bool`? i'm not sure if there's a reason for this to be an integer, when it seems like `1` is being used as `true` and `0` is being used as `false`. - i moved the nested matches so they are sequential. the second one will still only run if the first succeeds. - i moved `max_num_attempts` to be a static constant - i changed `two_seconds` to be named `sleep_duration`, might be a personal preference but imo the variable name should describe the purpose of what's in the container. so later when you wanna change how many seconds, you don't need to change that it's a `sleep_duration`. i'm curious, what bothers you about `match`? another approach (an alternative to the `while` loop) would be to use recusive functions, but i'm still not sure how to do that without `match`. if you plan to unwrap both cases (Ok and Err), afaik `match` is great for this. if you only want to unwrap Ok, then i'd use an `if let`. mayyybeee there's a way to do this with the `Result` methods? anyways, i hope this helped, happy to review pull requests anytime. 💜

👍 1

ahdinosaur commented

2022-07-12 11:19:58 +00:00

First-time contributor

for comparison, if we use ditch the Err cases and then use if let instead of match:

    while num_attempts < MAX_NUM_ATTEMPTS {
        let sbot_res = init_sbot().await;

        if let Ok(mut sbot) = sbot_res {
            let sbot_id_res = sbot.whoami().await;

            if let Ok(sbot_id) = sbot_id_res {
                whoami = Some(sbot_id);
                break;
            }
        }

        println!("trying again {:?}", num_attempts);
        num_attempts += 1;

        let sleep_duration = time::Duration::from_secs(2);
        thread::sleep(sleep_duration);
    }

for comparison, if we use ditch the `Err` cases and then use `if let` instead of `match`: ```rust while num_attempts < MAX_NUM_ATTEMPTS { let sbot_res = init_sbot().await; if let Ok(mut sbot) = sbot_res { let sbot_id_res = sbot.whoami().await; if let Ok(sbot_id) = sbot_id_res { whoami = Some(sbot_id); break; } } println!("trying again {:?}", num_attempts); num_attempts += 1; let sleep_duration = time::Duration::from_secs(2); thread::sleep(sleep_duration); } ```

👍 1

notplants commented

2022-07-12 11:33:15 +00:00

thanks @ahdinosaur ! much appreciated. I will comment in more detail later, but I've learned a bunch from these comments :)

notplants commented

2022-07-15 09:26:15 +00:00

if you only want to unwrap Ok, then i'd use an if let

nice to have clarity on when to use this, that makes sense!

i removed sbot_is_running and instead used a break to exit the loop. side note on sbot_is_running, shouldn't this have been a bool? i'm not sure if there's a reason for this to be an integer, when it seems like 1 is being used as true and 0 is being used as false.

cool, I didn't realize there was a break in rust. yes bool seems preferable, and removing all together to use break even better

i moved max_num_attempts to be a static constant

👍

i changed two_seconds to be named sleep_duration, might be a personal preference but imo the variable name should describe the purpose of what's in the container. so later when you wanna change how many seconds, you don't need to change that it's a sleep_duration.

👍

i'm curious, what bothers you about match?

there were some cases elsewhere where I was able to replace nested match statements with maps and transforms (the Result and Option methods), which made the code more concise and clear

so I don't specifically want to avoid match statements, but I found the nested match statements can get kind of deep and unwieldy.

I'm also not sure how to use the result methods within this specific while loop context, but using a mutable variable, and is_some() seems like an interesting pattern for doing things sequentially instead of in an ever-deepening nested match

> if you only want to unwrap Ok, then i'd use an if let nice to have clarity on when to use this, that makes sense! > i removed sbot_is_running and instead used a break to exit the loop. side note on sbot_is_running, shouldn't this have been a bool? i'm not sure if there's a reason for this to be an integer, when it seems like 1 is being used as true and 0 is being used as false. cool, I didn't realize there was a break in rust. yes bool seems preferable, and removing all together to use break even better > i moved max_num_attempts to be a static constant 👍 > i changed two_seconds to be named sleep_duration, might be a personal preference but imo the variable name should describe the purpose of what's in the container. so later when you wanna change how many seconds, you don't need to change that it's a sleep_duration. 👍 > i'm curious, what bothers you about match? there were some cases elsewhere where I was able to replace nested match statements with maps and transforms (the Result and Option methods), which made the code more concise and clear so I don't specifically want to avoid match statements, but I found the nested match statements can get kind of deep and unwieldy. I'm also not sure how to use the result methods within this specific while loop context, but using a mutable variable, and is_some() seems like an interesting pattern for doing things sequentially instead of in an ever-deepening nested match

notplants added 1 commit 2022-07-15 09:35:54 +00:00