-
Notifications
You must be signed in to change notification settings - Fork 157
improv(batch): improve errors #1648
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,59 @@ | ||
import { EventType } from './constants'; | ||
|
||
/** | ||
* Base error type for batch processing | ||
* All errors thrown by major failures extend this base class | ||
* Base error thrown by the Batch Processing utility | ||
*/ | ||
class BaseBatchProcessingError extends Error { | ||
public childErrors: Error[]; | ||
|
||
public msg: string; | ||
|
||
public constructor(msg: string, childErrors: Error[]) { | ||
super(msg); | ||
this.msg = msg; | ||
this.childErrors = childErrors; | ||
class BatchProcessingError extends Error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would we want the error thrown in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, very good catch. Updated in the latest commit. |
||
public constructor(message: string) { | ||
super(message); | ||
this.name = 'BatchProcessingError'; | ||
} | ||
} | ||
|
||
/** | ||
* Generates a list of errors that were generated by the major failure | ||
* @returns Formatted string listing all the errors that occurred | ||
* | ||
* @example | ||
* When all batch records fail to be processed, this will generate a string like: | ||
* All records failed processing. 3 individual errors logged separately below. | ||
* ,Failed to process record. | ||
* ,Failed to process record. | ||
* ,Failed to process record. | ||
*/ | ||
public formatErrors(parentErrorString: string): string { | ||
const errorList: string[] = [parentErrorString + '\n']; | ||
|
||
for (const error of this.childErrors) { | ||
errorList.push(error.message + '\n'); | ||
} | ||
/** | ||
* Error thrown by the Batch Processing utility when all batch records failed to be processed | ||
*/ | ||
class FullBatchFailureError extends BatchProcessingError { | ||
public recordErrors: Error[]; | ||
|
||
return '\n' + errorList; | ||
public constructor(childErrors: Error[]) { | ||
super('All records failed processing. See individual errors below.'); | ||
this.recordErrors = childErrors; | ||
this.name = 'FullBatchFailureError'; | ||
} | ||
} | ||
|
||
/** | ||
* When all batch records failed to be processed | ||
* Error thrown by the Batch Processing utility when a SQS FIFO queue is short-circuited. | ||
* This happens when a record fails processing and the remaining records are not processed | ||
* to avoid out-of-order delivery. | ||
*/ | ||
class BatchProcessingError extends BaseBatchProcessingError { | ||
public constructor(msg: string, childErrors: Error[]) { | ||
super(msg, childErrors); | ||
const parentErrorString: string = this.message; | ||
this.message = this.formatErrors(parentErrorString); | ||
class SqsFifoShortCircuitError extends BatchProcessingError { | ||
public constructor() { | ||
super( | ||
'A previous record failed processing. The remaining records were not processed to avoid out-of-order delivery.' | ||
); | ||
this.name = 'SqsFifoShortCircuitError'; | ||
} | ||
} | ||
|
||
export { BaseBatchProcessingError, BatchProcessingError }; | ||
/** | ||
* Error thrown by the Batch Processing utility when a partial processor receives an unexpected | ||
* batch type. | ||
*/ | ||
class UnexpectedBatchTypeError extends BatchProcessingError { | ||
public constructor() { | ||
super( | ||
`Unexpected batch type. Possible values are: ${Object.values( | ||
EventType | ||
).join(', ')}` | ||
); | ||
this.name = 'UnexpectedBatchTypeError'; | ||
} | ||
} | ||
export { | ||
BatchProcessingError, | ||
FullBatchFailureError, | ||
SqsFifoShortCircuitError, | ||
UnexpectedBatchTypeError, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to update the error passed to the failure handler here (and in
AsyncBatchProcessor
) to be theBatchProcessingError
as well?And potentially add tests for the error type checking like with the
SqsFifoPartialProcessor
tests if we add the typing here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should leave
Error
there because that's the type expected byfailureHandler
and we want that method to stay generic so that customers creating their own partial processor can pass any type of error. Making the error more strict would force them to have to subclass our own error class.