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

*: use axios-retry instead of handrolled retry methods #67

Closed
wants to merge 1 commit into from

Conversation

adityamaru
Copy link

@adityamaru adityamaru commented Dec 9, 2024

Important

Replace custom retry logic with axios-retry for HTTP requests in reporter.ts and setup_builder.ts.

  • Dependencies:
    • Add axios-retry to package.json dependencies.
    • Update typescript version in package.json.
  • HTTP Client Configuration:
    • Add createBlacksmithAPIClient and createBlacksmithAgentClient in reporter.ts to configure axios with axios-retry.
    • Use axios-retry for retry logic with 5 retries and exponential backoff.
  • Function Changes:
    • Replace postWithRetryToBlacksmithAPI and postWithRetry with axios-retry in reporter.ts.
    • Replace getWithRetry with get in reporter.ts.
    • Update getStickyDisk in setup_builder.ts to use createBlacksmithAgentClient.
  • Code Cleanup:
    • Remove utils.ts as it is no longer needed.

This description was created by Ellipsis for 810d280. It will automatically update as commits are pushed.

@adityamaru adityamaru changed the title WIP,DNM: use axios-retry for agent and API comms *: use axios-retry instead of handrolled retry methods Dec 9, 2024
@adityamaru adityamaru marked this pull request as ready for review December 9, 2024 15:11
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 50ccb6c in 1 minute and 46 seconds

More details
  • Looked at 430 lines of code in 4 files
  • Skipped 6 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. src/reporter.ts:26
  • Draft comment:
    The retry condition correctly checks for network or idempotent request errors and status codes >= 500. This ensures that retries are only attempted for recoverable errors.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The code uses axiosRetry to handle retries for network requests. However, the retry condition checks for status codes greater than or equal to 500, which is correct, but it should also consider network errors like timeouts or connection resets. The current implementation does this correctly, so no changes are needed here.
2. src/reporter.ts:34
  • Draft comment:
    Consider making stickyDiskMgrUrl configurable via an environment variable for flexibility.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The createBlacksmithAPIClient and createBlacksmithAgentClient functions are well-structured and correctly configure axios instances with retry logic. However, the createBlacksmithAgentClient function uses a hardcoded URL, which might be better as a configurable environment variable for flexibility.
3. src/reporter.ts:179
  • Draft comment:
    The get function is well-implemented, handling optional form data and signal options correctly.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The get function in reporter.ts is correctly implemented to handle GET requests with optional form data and signal options. It uses the axios instance correctly and sets the necessary headers.
4. src/setup_builder.ts:164
  • Draft comment:
    Consider adding error handling for unexpected response data in getStickyDisk.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    In setup_builder.ts, the getStickyDisk function uses the reporter.get function to make a GET request. The function is correctly implemented, but it might be beneficial to handle potential errors more explicitly, especially if the response data is not as expected.

Workflow ID: wflow_7ezzlhLXr9qj09Sx


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 810d280 in 27 seconds

More details
  • Looked at 414 lines of code in 4 files
  • Skipped 3 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. src/reporter.ts:21
  • Draft comment:
    Consider refactoring the axios-retry configuration into a separate function to avoid duplication in createBlacksmithAPIClient and createBlacksmithAgentClient.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The axios-retry configuration is duplicated in both createBlacksmithAPIClient and createBlacksmithAgentClient functions. This can be refactored to avoid redundancy.
2. src/reporter.ts:181
  • Draft comment:
    Ensure to handle potential errors in the get function to avoid unhandled promise rejections.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error handling in the get function should be improved to handle potential errors gracefully.
3. src/reporter.ts:34
  • Draft comment:
    Consider using an environment variable for stickyDiskMgrUrl instead of hardcoding it, to allow for more flexible configuration.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The createBlacksmithAgentClient function in reporter.ts and setup_builder.ts uses a hardcoded URL which might be better suited as a configurable environment variable.
4. src/setup_builder.ts:147
  • Draft comment:
    Consider using an environment variable for stickyDiskMgrUrl instead of hardcoding it, to allow for more flexible configuration.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The createBlacksmithAgentClient function in reporter.ts and setup_builder.ts uses a hardcoded URL which might be better suited as a configurable environment variable.

Workflow ID: wflow_NSM71zqPnGboew21


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@aayushshah15
Copy link

We need to rebase this I think @adityamaru

@adityamaru adityamaru closed this Dec 9, 2024
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