-
Notifications
You must be signed in to change notification settings - Fork 31
feat: adds polling synchronizer support #816
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: adds polling synchronizer support #816
Conversation
…dge cases. Added tests.
| if (options.wrapper.version) { | ||
| cf.wrapperVersion = options.wrapper.version; | ||
| } | ||
| } |
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.
For reviewers: Hooks up configuration for contract tests.
| streaming/fdv2/reconnection state management/replaces previously known state | ||
| streaming/fdv2/reconnection state management/updates previously known state | ||
| streaming/fdv2/ignores model version | ||
| streaming/fdv2/can discard partial events on errors |
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.
These suppressions are the fdv2 cases that aren't passing at moment. The state and model ones will be addressed in a future story.
The streaming/fdv2/reconnection state management/initializes from polling initializer one is trickier. The JS configuration does not support the same level of initializer customization that the test harness and Go SDK support, so those cases are not supported.
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.
For reviewers: This file change generalizes handling to be agnostic of streaming so that polling and streaming can share the same core logic. It is a rename and then some tweaks.
| id: string; | ||
| version: number; | ||
| state: string; | ||
| state?: string; |
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.
For reviewers: This state is now optional as part of a fix to support the "transfer none" case where the server does not give us a state value.
| this._attachHandler(eventStream, 'delete-object', this._processDeleteObject); | ||
| this._attachHandler(eventStream, 'payload-transferred', this._processPayloadTransferred); | ||
| this._attachHandler(eventStream, 'goodbye', this._processGoodbye); | ||
| this._attachHandler(eventStream, 'error', this._processError); |
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.
For reviewers: This logic moved to the streaming specific PayloadStreamReader.
| `Stream received data that was unable to be processed in "${eventName}" message`, | ||
| ); | ||
| this._logger?.debug(`Data follows: ${event.data}`); | ||
| this._errorHandler?.(DataSourceErrorKind.InvalidData, 'Malformed data in event stream'); |
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.
For reviewers: This logic moved to the streaming specific PayloadStreamReader.
|
|
||
| this._listeners.forEach((it) => it(payload)); | ||
| this._resetAfterEmission(); | ||
| }; |
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.
For reviewers: Transfer none wasn't emitting an empty payload, now that is fixed. The empty payload signals to other layers of the SDK that communication has occurred.
| private _resetAll() { | ||
| this._tempId = undefined; | ||
| this._tempBasis = undefined; | ||
| this._tempBasis = false; |
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.
For reviewers: Various tweaks to support transfer none case and error cases.
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.
For reviewers: This logic existed already, but teased apart the streaming specific concerns from the payload processing concerns.
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.
For reviewers: Tweaks to hook into the composite data source since the CompositeDataSource is now being used with Streaming and Polling.
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.
For reviewers: Noticed some seconds to millis conversions were missing here.
Requirements
I have added test coverage for new or changed functionality
I have followed the repository's pull request submission guidelines
I have validated my changes against all supported platform versions
This will be done at a later time on the target branch during integration testing.
Related issues
SDK-858 and SDK-851
Describe the solution you've provided
Refactors payload processing to be reusable between polling data sources and streaming data sources.
Adds OneShotInitializers and PollingSynchronizer.
Adds support to contract tests for sdk-test-harness/feat/fdv2 branch.