feat: CompositeDataSource blacklists DataSources that report unrecoverable error #826
Conversation
| expect(statusCallback).toHaveBeenCalledTimes(4); | ||
| }); | ||
|
|
||
| // it('it reports DataSourceState Off when all synchronizers report Off', async () => { |
There was a problem hiding this comment.
For reviewers: Still tweaking this test case.
There was a problem hiding this comment.
CompositeDataSource is now stitching together status of underlying datasources into a cohesive series, so most of the changes in here are to enhance the existing tests to check the status sequences.
| expect(statusCallback).toHaveBeenNthCalledWith(1, DataSourceState.Initializing, undefined); | ||
| expect(statusCallback).toHaveBeenNthCalledWith(2, DataSourceState.Valid, undefined); | ||
| expect(statusCallback).toHaveBeenNthCalledWith(3, DataSourceState.Interrupted, undefined); | ||
| expect(statusCallback).toHaveBeenNthCalledWith(4, DataSourceState.Valid, undefined); |
There was a problem hiding this comment.
For reviewers: Here it is now checking the top level status updates are a proper merging of the underlying data source statuses.
| */ | ||
| stop(): void; | ||
| } | ||
|
|
There was a problem hiding this comment.
For reviewers: The initializer and synchronizer factory distinction was making writing the logic related to culling a blacklist datasource cumbersome. So removed that distinction to make the logic the same for both types.
| // Data source was closed and will not retry | ||
| Closed, | ||
| // This datasource encountered an unrecoverable error and it is not expected to be resolved through trying again in the future | ||
| Off, |
There was a problem hiding this comment.
For reviewers: Added Off and reordered so Valid would be 0 (positive result).
There was a problem hiding this comment.
Is that considered a breaking change then?
| * Represents a transition between data sources. | ||
| */ | ||
| export type Transition = 'none' | 'switchToSync' | 'fallback' | 'recover' | 'stop'; | ||
| export type Transition = 'switchToSync' | 'fallback' | 'recover' | 'stop'; |
There was a problem hiding this comment.
Undefined transition takes the place of none now.
| } | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
For reviewers: All the position tracking logic is now in the dataSourceList class to encapsulate iteration.
| if (!condition || (excludeRecover && condition.transition === 'recover')) { | ||
| if (excludeRecover && condition?.transition === 'recover') { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
For reviewers: No logical change intended here.
| subsystemCommon.DataSourceState.Closed, | ||
| subsystemCommon.DataSourceState.Off, | ||
| new LDPollingError(DataSourceErrorKind.ErrorResponse, message, status), | ||
| ); |
There was a problem hiding this comment.
For reviewers: This is what leads to blacklisting after unrecoverable error.
keelerm84
left a comment
keelerm84
left a comment
There was a problem hiding this comment.
Looks good overall. Not approving since there is a pending test commented out, but otherwise 👍🏼
| // Data source was closed and will not retry | ||
| Closed, | ||
| // This datasource encountered an unrecoverable error and it is not expected to be resolved through trying again in the future | ||
| Off, |
There was a problem hiding this comment.
Is that considered a breaking change then?
tanderson-ld
changed the title
Addressed! |
Requirements
To be done on target branch.