feat: CompositeDataSource blacklists DataSources that report unrecoverable error #826
Conversation
| }); | ||
|
|
||
| // 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.
| @@ -28,16 +30,4 @@ export interface DataSource { | |||
| 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.
| 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?
|
|
||
| const DEFAULT_FALLBACK_TIME_MS = 2 * 60 * 1000; | ||
| const DEFAULT_RECOVERY_TIME_MS = 5 * 60 * 1000; | ||
|
|
||
| /** | ||
| * 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.
| } else { | ||
| isPrimary = this._syncFactories.pos() === 0; | ||
| factory = this._syncFactories.next(); | ||
| } | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
For reviewers: All the position tracking logic is now in the dataSourceList class to encapsulate iteration.
| @@ -273,7 +305,7 @@ export class CompositeDataSource implements DataSource { | |||
| const condition = this._transitionConditions[state]; | |||
|
|
|||
| // exclude recovery can happen for certain initializers/synchronizers (ex: the primary synchronizer shouldn't recover to itself) | |||
| 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.
| @@ -34,7 +34,7 @@ export default class OneShotInitializerFDv2 implements subsystemCommon.DataSyste | |||
| const message = httpErrorMessage(err, 'initializer', 'initializer does not retry'); | |||
| this._logger?.error(message); | |||
| statusCallback( | |||
| 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.
| 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.