-
Notifications
You must be signed in to change notification settings - Fork 83
[FSSDK-11003] disposable service implementation #981
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 4 commits
060cbc3
11c850d
17d27a3
e516e39
08d0df4
bad40b4
f2c3b5a
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 |
|---|---|---|
|
|
@@ -30,6 +30,9 @@ import { areEventContextsEqual } from "./event_builder/user_event"; | |
| import { EVENT_PROCESSOR_STOPPED, FAILED_TO_DISPATCH_EVENTS, FAILED_TO_DISPATCH_EVENTS_WITH_ARG } from "../exception_messages"; | ||
| import { sprintf } from "../utils/fns"; | ||
|
|
||
| export const DEFAULT_MIN_BACKOFF = 1000; | ||
| export const DEFAULT_MAX_BACKOFF = 32000; | ||
|
|
||
| export type EventWithId = { | ||
| id: string; | ||
| event: ProcessableEvent; | ||
|
|
@@ -209,7 +212,8 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
| if (!batch) { | ||
| return; | ||
| } | ||
|
|
||
|
|
||
| this.dispatchRepeater.reset(); | ||
| this.dispatchBatch(batch, closing); | ||
| } | ||
|
|
||
|
|
@@ -218,10 +222,6 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
| return Promise.reject('Event processor is not running'); | ||
| } | ||
|
|
||
| if (this.eventQueue.length == this.batchSize) { | ||
| this.flush(); | ||
| } | ||
|
|
||
| const eventWithId = { | ||
| id: this.idGenerator.getId(), | ||
| event: event, | ||
|
|
@@ -232,13 +232,30 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
| if (this.eventQueue.length > 0 && !areEventContextsEqual(this.eventQueue[0].event, event)) { | ||
| this.flush(); | ||
| } | ||
| this.eventQueue.push(eventWithId); | ||
|
|
||
| this.eventQueue.push(eventWithId); | ||
|
|
||
| if (this.eventQueue.length == this.batchSize) { | ||
| this.flush(); | ||
| } else if (!this.dispatchRepeater.isRunning()) { | ||
| this.dispatchRepeater.start(); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| start(): void { | ||
| if (!this.isNew()) { | ||
| return; | ||
| } | ||
| if(this.disposable) { | ||
|
||
| this.batchSize = 1; | ||
| this.retryConfig = { | ||
| maxRetries: Math.min(this.retryConfig?.maxRetries ?? 5, 5), | ||
| backoffProvider: | ||
| this.retryConfig?.backoffProvider || | ||
| (() => new ExponentialBackoff(DEFAULT_MIN_BACKOFF, DEFAULT_MAX_BACKOFF, 500)), | ||
| }; | ||
| } | ||
|
||
| super.start(); | ||
| this.state = ServiceState.Running; | ||
| this.dispatchRepeater.start(); | ||
|
|
@@ -254,7 +271,6 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
| } | ||
|
|
||
| if (this.isNew()) { | ||
| // TOOD: replace message with imported constants | ||
| this.startPromise.reject(new Error(EVENT_PROCESSOR_STOPPED)); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,6 +207,39 @@ describe('DefaultOdpEventManager', () => { | |
| } | ||
| }); | ||
|
|
||
| it('should flush the queue immediately if disposable, regardless of the batchSize', async () => { | ||
| const apiManager = getMockApiManager(); | ||
| const repeater = getMockRepeater() | ||
| apiManager.sendEvents.mockResolvedValue({ statusCode: 200 }); | ||
| // spy on the flush method | ||
| const odpEventManager = new DefaultOdpEventManager({ | ||
| repeater, | ||
| apiManager: apiManager, | ||
| batchSize: 10, | ||
| retryConfig: { | ||
| maxRetries: 3, | ||
| backoffProvider: vi.fn(), | ||
| }, | ||
| }); | ||
|
|
||
| odpEventManager.updateConfig({ | ||
| integrated: true, | ||
| odpConfig: config, | ||
| }); | ||
| odpEventManager.makeDisposable(); | ||
| odpEventManager.start(); | ||
|
|
||
| await expect(odpEventManager.onRunning()).resolves.not.toThrow(); | ||
|
|
||
| const event = makeEvent(0); | ||
| odpEventManager.sendEvent(event); | ||
| await exhaustMicrotasks(); | ||
|
|
||
| expect(apiManager.sendEvents).toHaveBeenCalledTimes(1); | ||
| expect(apiManager.sendEvents).toHaveBeenNthCalledWith(1, config, [event]); | ||
| expect(repeater.reset).toHaveBeenCalledTimes(1); | ||
| }) | ||
|
|
||
|
Contributor
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. can we also add tests for repeater stop when disposable = true?
Contributor
Author
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. In the actual implementation
Contributor
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. Oh I see we have assertions for repeater.reset here. We can ignore this comment
Contributor
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. we can add a test to assert the repeater is not started, similar to eventProcessor |
||
| it('drops events and logs if the state is not running', async () => { | ||
| const apiManager = getMockApiManager(); | ||
| apiManager.sendEvents.mockResolvedValue({ statusCode: 200 }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,11 @@ export class DefaultOdpEventManager extends BaseService implements OdpEventManag | |
| if (!this.isNew) { | ||
| return; | ||
| } | ||
| // Override for disposable event manager | ||
| if(this.disposable) { | ||
|
||
| this.retryConfig.maxRetries = Math.min(this.retryConfig.maxRetries, 5); | ||
| this.batchSize = 1 | ||
| } | ||
|
|
||
| super.start(); | ||
| if (this.odpIntegrationConfig) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,7 @@ const getMockOdpEventManager = () => { | |
| getState: vi.fn(), | ||
| updateConfig: vi.fn(), | ||
| sendEvent: vi.fn(), | ||
| makeDisposable: vi.fn(), | ||
|
Contributor
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. can we add test that it makes the eventManager disposable when its makeDisposable() is called? |
||
| }; | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,10 @@ export class DefaultOdpManager extends BaseService implements OdpManager { | |
| if (!this.isNew()) { | ||
| return; | ||
| } | ||
|
|
||
| if(this.disposable) { | ||
|
||
| this.eventManager.makeDisposable(); | ||
| } | ||
|
|
||
| this.state = ServiceState.Starting; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,10 @@ | |
| import { describe, it, expect, vi } from 'vitest'; | ||
| import Optimizely from '.'; | ||
| import { getMockProjectConfigManager } from '../tests/mock/mock_project_config_manager'; | ||
| import * as logger from '../plugins/logger'; | ||
| import * as jsonSchemaValidator from '../utils/json_schema_validator'; | ||
| import { LOG_LEVEL } from '../common_exports'; | ||
| import { createNotificationCenter } from '../notification_center'; | ||
| import testData from '../tests/test_data'; | ||
| import { getForwardingEventProcessor } from '../event_processor/forwarding_event_processor'; | ||
| import { LoggerFacade } from '../modules/logging'; | ||
| import { createProjectConfig } from '../project_config/project_config'; | ||
| import { getMockLogger } from '../tests/mock/mock_logger'; | ||
|
|
||
|
|
@@ -39,12 +36,12 @@ describe('Optimizely', () => { | |
|
|
||
| const notificationCenter = createNotificationCenter({ logger, errorHandler }); | ||
|
|
||
| it('should pass ssr to the project config manager', () => { | ||
| it('should pass disposable option to the project config manager', () => { | ||
|
||
| const projectConfigManager = getMockProjectConfigManager({ | ||
| initConfig: createProjectConfig(testData.getTestProjectConfig()), | ||
| }); | ||
|
|
||
| vi.spyOn(projectConfigManager, 'setSsr'); | ||
| vi.spyOn(projectConfigManager, 'makeDisposable'); | ||
|
|
||
| const instance = new Optimizely({ | ||
| clientEngine: 'node-sdk', | ||
|
|
@@ -54,16 +51,16 @@ describe('Optimizely', () => { | |
| logger, | ||
| notificationCenter, | ||
| eventProcessor, | ||
| isSsr: true, | ||
| disposable: true, | ||
| isValidInstance: true, | ||
| }); | ||
|
|
||
| expect(projectConfigManager.setSsr).toHaveBeenCalledWith(true); | ||
| expect(projectConfigManager.makeDisposable).toHaveBeenCalled(); | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| expect(instance.getProjectConfig()).toBe(projectConfigManager.config); | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| expect(projectConfigManager.isSsr).toBe(true); | ||
| expect(projectConfigManager.disposable).toBe(true); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -470,6 +470,25 @@ describe('PollingDatafileManager', () => { | |
| expect(repeater.stop).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('stops repeater after successful initialization if disposable is true', async () => { | ||
| const repeater = getMockRepeater(); | ||
| const requestHandler = getMockRequestHandler(); | ||
| const mockResponse = getMockAbortableRequest(Promise.resolve({ statusCode: 200, body: '{"foo": "bar"}', headers: {} })); | ||
| requestHandler.makeRequest.mockReturnValueOnce(mockResponse); | ||
|
|
||
| const manager = new PollingDatafileManager({ | ||
| repeater, | ||
| requestHandler, | ||
| sdkKey: 'keyThatExists', | ||
| }); | ||
| manager.makeDisposable(); | ||
| manager.start(); | ||
| repeater.execute(0); | ||
|
|
||
| await expect(manager.onRunning()).resolves.not.toThrow(); | ||
| expect(repeater.stop).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
|
Contributor
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. can we also add tests that stops retrying to initialize after 5 failed attempts if disposable is true? |
||
| it('saves the datafile in cache', async () => { | ||
| const repeater = getMockRepeater(); | ||
| const requestHandler = getMockRequestHandler(); | ||
|
|
||
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.
can we add tests for the following as well when disposable
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.
Regarding your number 2 - Current logic is -
dispatchRepeaterstarts, dispatch immediately and then stops. Isn't this expected ? Or am I missing something 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.
according to the refactored logic in this PR, if batchSize == 1, dispatch repeater should never start. It will immediately dispatch the event, the repeater start is in the else branch