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

feat: error retry handling #49

Merged
merged 3 commits into from
Jan 7, 2025
Merged

feat: error retry handling #49

merged 3 commits into from
Jan 7, 2025

Conversation

0xnigir1
Copy link
Collaborator

@0xnigir1 0xnigir1 commented Jan 6, 2025

🤖 Linear

Closes GIT-223 GIT-224 GIT-110

Description

We implement a RetryHandler with ExponentialBackoff for handling error retries (centralized on the Orchestrator) and define two big base class of errors:

  • Retriable
  • NonRetriable

In the next PR, will adjust the different errors of the system and packages to extend from this classes accordingly

Checklist before requesting a review

  • I have conducted a self-review of my code.
  • I have conducted a QA.
  • If it is a core feature, I have included comprehensive tests.

@0xnigir1 0xnigir1 requested review from jahabeebs and 0xyaco January 6, 2025 20:03
@0xnigir1 0xnigir1 self-assigned this Jan 6, 2025
Copy link
Collaborator

@jahabeebs jahabeebs left a comment

Choose a reason for hiding this comment

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

Nice! 👌

@@ -36,6 +36,10 @@ const baseSchema = z.object({
IPFS_GATEWAYS_URL: stringToJSONSchema
.pipe(z.array(z.string().url()))
.default('["https://ipfs.io"]'),
RETRY_MAX_ATTEMPTS: z.coerce.number().int().min(1).default(3),
RETRY_BASE_DELAY_MS: z.coerce.number().int().min(1).default(3000), // 3 seconds
RETRY_FACTOR: z.coerce.number().int().min(1).default(2),
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to mark these three as optional within schema chaining since they're technically not required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with the default() they are optional, you are not forced to pass them

@@ -0,0 +1,40 @@
export interface ErrorContext {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the future it would be good to implement more specific error contexts depending on the class/functions throwing the errors so that you don't have a ton of optional params (maybe a bit too generic)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could be, i wanted to keep it simple for now

Comment on lines +17 to +19
* @param options.baseDelay - Initial delay in milliseconds (default: 5000)
* @param options.factor - Multiplier for exponential increase (default: 2)
* @param options.maxAttempts - Maximum number of retry attempts (default: 3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are technically not required though right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, i mean you can avoid passing the options object and have the default being set

* - Registry tracking of supported/unsupported strategies and events
*
* TODO: Enhance the error handling/retries, logging and observability
Copy link
Collaborator

Choose a reason for hiding this comment

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

*/
private addJitter(delay: number): number {
// Random value between 0.8 and 1.2
const jitterFactor = 0.8 + Math.random() * 0.4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the delta for 1 ± delta be configurable too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could be, will add it to Linear as improvement (isn't critical now)

@0xnigir1 0xnigir1 merged commit 36f8638 into dev Jan 7, 2025
6 checks passed
@0xnigir1 0xnigir1 deleted the feat/error-retry-handling branch January 7, 2025 12: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.

3 participants