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

Daemonize rpc-downloader and importer-offline #1854

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

arthurmm-cloudwalk
Copy link
Contributor

@arthurmm-cloudwalk arthurmm-cloudwalk commented Nov 7, 2024

User description

Turns rpc_downloader and importer_offline as daemon, so they can keep running unstoppable


PR Type

Enhancement


Description

  • Implemented daemon mode for both importer_offline and rpc_downloader, allowing them to run continuously and process new blocks.
  • Added configuration options for daemon mode and interval in src/config.rs.
  • Enhanced importer_offline to support continuous block processing in daemon mode, with error handling and shutdown mechanisms.
  • Modified rpc_downloader to support continuous downloading of blocks and balances in daemon mode.
  • Improved error handling and added graceful shutdown capabilities for both binaries.

Changes walkthrough 📝

Relevant files
Enhancement
importer_offline.rs
Implement daemon mode for importer_offline                             

src/bin/importer_offline.rs

  • Added daemon mode functionality to the external RPC storage loader
  • Implemented continuous block processing in daemon mode
  • Added error handling and shutdown mechanisms
  • +56/-38 
    rpc_downloader.rs
    Add daemon mode to rpc_downloader                                               

    src/bin/rpc_downloader.rs

  • Added daemon mode to continuously download blocks and balances
  • Implemented a loop to restart the download process after a wait period
  • Added shutdown handling for daemon mode
  • +22/-7   
    Configuration changes
    config.rs
    Add daemon mode configuration options                                       

    src/config.rs

  • Added daemon mode configuration option for RpcDownloaderConfig
  • Added daemon mode and interval configuration options for
    ImporterOfflineConfig
  • +12/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    github-actions bot commented Nov 7, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Memory Leak
    The infinite loop in the daemon mode might cause unbounded growth of the tasks vector if new blocks are continuously added faster than they can be processed.

    Inefficient Sleep
    The use of tokio::time::sleep in the daemon mode should be replaced with traced_sleep for better visibility in tracing events.

    Missing Span Fields
    The tracing::info! macro calls are not recording any identifiers as span fields, which could be useful for debugging and monitoring.

    Copy link

    github-actions bot commented Nov 7, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract repetitive logic into separate functions to improve code organization and readability

    Consider extracting the download logic into a separate function to improve code
    readability and maintainability. This would make the main loop cleaner and easier to
    understand.

    src/bin/rpc_downloader.rs [51-73]

    -loop {
    +async fn perform_download_cycle(rpc_storage: &Arc<dyn ExternalRpcStorage>, chain: &Arc<BlockchainClient>, config: &RpcDownloaderConfig) -> anyhow::Result<()> {
         let block_end = match config.block_end {
             Some(end) => BlockNumber::from(end),
             None => chain.fetch_block_number().await?,
         };
     
    -    // download balances and blocks
    -    download_balances(Arc::clone(&rpc_storage), &chain, config.initial_accounts.clone()).await?;
    +    download_balances(Arc::clone(rpc_storage), chain, config.initial_accounts.clone()).await?;
         download_blocks(rpc_storage.clone(), chain.clone(), config.paralellism, block_end).await?;
     
    -    if !config.daemon {
    +    Ok(())
    +}
    +
    +loop {
    +    perform_download_cycle(&rpc_storage, &chain, &config).await?;
    +
    +    if !config.daemon || GlobalState::is_shutdown_warn("rpc-downloader::main") {
             break;
         }
     
    -    if GlobalState::is_shutdown_warn("rpc-downloader::main") {
    -        break;
    -    }
    -
    -    // wait for 1 minute before restarting the download
         let wait_duration = Duration::from_secs(60);
         tracing::info!("Daemon mode enabled, waiting {} seconds before restarting...", wait_duration.as_secs());
         tokio::time::sleep(wait_duration).await;
     }
    Suggestion importance[1-10]: 7

    Why: Extracting the download logic into a separate function would significantly improve code readability and maintainability, making the main loop cleaner and easier to understand.

    7
    Best practice
    Use a constant for time intervals instead of hardcoded values to improve code maintainability

    Consider using a constant for the daemon interval instead of hardcoding the value of
    60 seconds. This would improve maintainability and make it easier to adjust the
    interval in the future.

    src/bin/importer_offline.rs [229]

    -tokio::time::sleep(Duration::from_secs(daemon_interval)).await;
    +const DAEMON_INTERVAL_SECS: u64 = 60;
    +tokio::time::sleep(Duration::from_secs(DAEMON_INTERVAL_SECS)).await;
    Suggestion importance[1-10]: 2

    Why: While using a constant can improve maintainability, the PR already introduces a configurable 'daemon_interval' parameter, making this suggestion less impactful.

    2
    Use constants for time durations instead of hardcoded values to improve code maintainability

    Consider using a constant for the wait duration instead of hardcoding the value of
    60 seconds. This would improve maintainability and make it easier to adjust the
    interval in the future.

    src/bin/rpc_downloader.rs [70]

    -let wait_duration = Duration::from_secs(60);
    +const DAEMON_WAIT_DURATION_SECS: u64 = 60;
    +let wait_duration = Duration::from_secs(DAEMON_WAIT_DURATION_SECS);
    Suggestion importance[1-10]: 2

    Why: While using a constant can improve maintainability, the hardcoded value is used only once in this context, making the impact of this change relatively low.

    2

    @arthurmm-cloudwalk
    Copy link
    Contributor Author

    /benchmark

    @gabriel-aranha-cw
    Copy link
    Contributor

    Benchmark:
    Run ID: bench-1677534657

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1005.00, Min: 650.00, Avg: 822.13, StdDev: 52.47
    TPS Stats: Max: 937.00, Min: 701.00, Avg: 796.68, StdDev: 59.93

    Plot: View Plot

    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.

    2 participants