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

fix: backoff before retrieving receipts #926

Merged
merged 1 commit into from
May 24, 2024
Merged

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner May 24, 2024 20:45
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the changes are minimal and localized to a specific functionality, but understanding the impact of timing changes in asynchronous operations requires careful consideration.

🧪 Relevant tests

No

⚡ Possible issues

Possible Race Condition: Introducing a fixed delay (BACKOFF_RECEIPTS) might not be sufficient or optimal for all network conditions or block sizes. This could lead to situations where receipts are still not available after the delay, especially under high network load or large blocks.

🔒 Security concerns

No

Code feedback:
relevant filesrc/bin/importer_online.rs
suggestion      

Consider implementing a dynamic backoff strategy or using an exponential backoff to handle different network conditions and block sizes more efficiently. This could help in reducing unnecessary delays or handling high load situations better. [important]

relevant lineconst BACKOFF_RECEIPTS: Duration = Duration::from_millis(10);

relevant filesrc/bin/importer_online.rs
suggestion      

Evaluate the use of a configurable parameter for BACKOFF_RECEIPTS instead of a hard-coded value. This allows for easier adjustments based on deployment environment or network conditions without needing to change the code. [important]

relevant lineconst BACKOFF_RECEIPTS: Duration = Duration::from_millis(10);

relevant filesrc/bin/importer_online.rs
suggestion      

Add logging before and after the sleep to trace the behavior and effectiveness of the backoff in production. This can help in debugging issues related to timing and receipt fetching. [medium]

relevant linelet _ = tokio::time::sleep(BACKOFF_RECEIPTS).await;

relevant filesrc/bin/importer_online.rs
suggestion      

Consider checking the availability of receipts in a loop with a timeout to ensure that you proceed only when the receipts are actually available, thus avoiding potential failures in fetching them. [important]

relevant linelet _ = tokio::time::sleep(BACKOFF_RECEIPTS).await;

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Implement a dynamic backoff strategy for better handling of receipt availability

Instead of using a hardcoded sleep, consider implementing a backoff strategy that
dynamically adjusts based on the availability of receipts. This could involve checking for
receipt availability and increasing the wait time exponentially if they are not yet
available.

src/bin/importer_online.rs [292]

-let _ = tokio::time::sleep(BACKOFF_RECEIPTS).await;
+let mut backoff = BACKOFF_RECEIPTS;
+while receipts_not_available() {
+    tokio::time::sleep(backoff).await;
+    backoff *= 2; // Exponential backoff
+}
 
Suggestion importance[1-10]: 9

Why: Implementing a dynamic backoff strategy can significantly enhance the robustness and efficiency of the receipt retrieval process by adapting to varying conditions, making it a valuable improvement.

9
Add error handling for the sleep operation to enhance robustness

Add error handling for the sleep operation to manage scenarios where the sleep might be
interrupted or fail. This can help in ensuring the robustness of the receipt fetching
process.

src/bin/importer_online.rs [292]

-let _ = tokio::time::sleep(BACKOFF_RECEIPTS).await;
+if let Err(e) = tokio::time::sleep(BACKOFF_RECEIPTS).await {
+    log::error!("Failed to sleep for receipt backoff: {}", e);
+    return Err(e);
+}
 
Suggestion importance[1-10]: 8

Why: Adding error handling for the sleep operation enhances the robustness of the code by ensuring that potential interruptions or failures are managed properly, which is important for reliable execution.

8
Improve the clarity of the constant name by making it more descriptive

Consider using a more descriptive variable name for BACKOFF_RECEIPTS to reflect its
purpose more clearly. For example, RECEIPT_RETRIEVAL_BACKOFF would indicate that this
constant is specifically for delaying the retrieval of receipts.

src/bin/importer_online.rs [54]

-const BACKOFF_RECEIPTS: Duration = Duration::from_millis(10);
+const RECEIPT_RETRIEVAL_BACKOFF: Duration = Duration::from_millis(10);
 
Suggestion importance[1-10]: 7

Why: The suggestion improves code readability and maintainability by making the constant name more descriptive, but it does not address a critical issue.

7
Possible issue
Increase the backoff time to ensure the availability of receipts

Increase the backoff time for receipt retrieval to ensure that receipts have adequate time
to become available. A backoff time of 10 milliseconds may be too short. Consider
increasing it to at least 100 milliseconds.

src/bin/importer_online.rs [54]

-const BACKOFF_RECEIPTS: Duration = Duration::from_millis(10);
+const BACKOFF_RECEIPTS: Duration = Duration::from_millis(100);
 
Suggestion importance[1-10]: 8

Why: This suggestion addresses a potential issue where the current backoff time might be insufficient, potentially leading to errors in receipt retrieval. Increasing the backoff time can improve the reliability of the process.

8

@renancloudwalk renancloudwalk merged commit 8359ec2 into main May 24, 2024
31 checks passed
@renancloudwalk renancloudwalk deleted the fix-importer-online-2 branch May 24, 2024 21:00
Copy link
Contributor

@renancloudwalk renancloudwalk left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants