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

enha: additional checks in importer-offline #1547

Merged
merged 1 commit into from
Jul 25, 2024
Merged

enha: additional checks in importer-offline #1547

merged 1 commit into from
Jul 25, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 25, 2024

PR Type

Enhancement, Configuration changes


Description

  • Added validation to ensure block ranges are not empty and do not have gaps in src/bin/importer_offline.rs.
  • Improved logging messages for better traceability in src/bin/importer_offline.rs.
  • Allowed unwrap usage in tests by updating .clippy.toml.
  • Allowed unwrap usage in the project by updating Cargo.toml.

Changes walkthrough 📝

Relevant files
Enhancement
importer_offline.rs
Add validation and logging improvements in block importer

src/bin/importer_offline.rs

  • Added checks for empty block ranges and gaps in block ranges.
  • Improved logging messages for better traceability.
  • Renamed variables for clarity.
  • +24/-11 
    Configuration changes
    .clippy.toml
    Allow unwrap usage in tests in Clippy configuration           

    .clippy.toml

    • Allowed unwrap in tests.
    +1/-0     
    Cargo.toml
    Allow unwrap usage in Cargo configuration                               

    Cargo.toml

    • Allowed unwrap usage in the project.
    +2/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Unwrap Usage
    The code uses unwrap in several places. It is recommended to use expect with a meaningful error message to provide better context in case of a panic.

    Missing Span Fields
    The function execute_block_importer is instrumented with tracing::info, but it does not record identifiers as span fields. Consider adding relevant identifiers to the tracing span for better traceability.

    Background Task Logging
    When spawning background tasks with tokio::spawn or tokio::spawn_blocking, ensure that the operations are properly logged and monitored. Consider using spawn_named or spawn_blocking_named for better traceability.

    Copy link

    github-actions bot commented Jul 25, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use pattern matching instead of unwrap() to handle potential errors gracefully

    Instead of using unwrap() to get the block numbers, consider using pattern matching
    to handle potential errors gracefully.

    src/bin/importer_offline.rs [135-136]

    -let block_start = blocks.first().unwrap().number();
    -let block_end = blocks.last().unwrap().number();
    +let (Some(block_start), Some(block_end)) = (blocks.first(), blocks.last()) else {
    +    let message = GlobalState::shutdown_from(TASK_NAME, "received empty block range to reexecute");
    +    return log_and_err!(message);
    +};
    +let block_start = block_start.number();
    +let block_end = block_end.number();
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly replaces unwrap() with pattern matching, which is a safer and more idiomatic way to handle potential errors in Rust. This change improves the robustness of the code.

    9
    Performance
    Add a timeout to the blocking_recv call to avoid potential indefinite blocking

    Add a timeout to the blocking_recv call to avoid potential indefinite blocking.

    src/bin/importer_offline.rs [123-125]

    -let Some((blocks, receipts)) = backlog_rx.blocking_recv() else {
    +let Some((blocks, receipts)) = backlog_rx.recv_timeout(Duration::from_secs(30)) else {
         tracing::info!("{} has no more blocks to reexecute", TASK_NAME);
         return Ok(());
     };
     
    Suggestion importance[1-10]: 8

    Why: Adding a timeout to the blocking_recv call is a good practice to prevent potential indefinite blocking, which can improve the performance and reliability of the system.

    8
    Enhancement
    Simplify the load_blocks_and_receipts function by using try_join! macro directly in the return statement

    Use try_join! macro directly in the return statement to simplify the
    load_blocks_and_receipts function.

    src/bin/importer_offline.rs [248-250]

    -let blocks_task = rpc_storage.read_blocks_in_range(block_start, block_end);
    -let receipts_task = rpc_storage.read_receipts_in_range(block_start, block_end);
    -try_join!(blocks_task, receipts_task)
    +try_join!(
    +    rpc_storage.read_blocks_in_range(block_start, block_end),
    +    rpc_storage.read_receipts_in_range(block_start, block_end)
    +)
     
    Suggestion importance[1-10]: 7

    Why: The suggestion simplifies the code by using the try_join! macro directly in the return statement, which enhances readability and maintainability without changing the functionality.

    7

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 25, 2024 20:21
    @dinhani-cw dinhani-cw merged commit de5d54c into main Jul 25, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the unwrap branch July 25, 2024 20:25
    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.

    1 participant