Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit c3b6ce7

Browse files
mjc1283mjc1283
authored
fix (datafile manager): Small cleanups in datafile manager (optimizely#17)
Summary: - Only assign current datafile if truthy (replace typeof datafile !== 'undefined' check with datafile check) - Change urlTemplate option to be a sprintf-style format string, like Java's withFormat - Change backoff delay to increase up to 512 seconds after 7 errors, and clean up the backoff implementation a little - Clean up url handling in HttpPollingConfigManager. It was saving more private variables than necessary for data that never changes after the constructor. Test plan: Updated unit tests
1 parent c987725 commit c3b6ce7

File tree

6 files changed

+49
-40
lines changed

6 files changed

+49
-40
lines changed

‎packages/datafile-manager/__test__/backoffController.spec.ts

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,6 @@ describe('backoffController', () => {
2626
it('increases the delay returned from getDelay (up to a maximum value) after each call to countError', () => {
2727
const controller = new BackoffController()
2828
controller.countError()
29-
expect(controller.getDelay()).toBeGreaterThanOrEqual(2000)
30-
expect(controller.getDelay()).toBeLessThan(3000)
31-
controller.countError()
32-
expect(controller.getDelay()).toBeGreaterThanOrEqual(4000)
33-
expect(controller.getDelay()).toBeLessThan(5000)
34-
controller.countError()
3529
expect(controller.getDelay()).toBeGreaterThanOrEqual(8000)
3630
expect(controller.getDelay()).toBeLessThan(9000)
3731
controller.countError()
@@ -40,10 +34,22 @@ describe('backoffController', () => {
4034
controller.countError()
4135
expect(controller.getDelay()).toBeGreaterThanOrEqual(32000)
4236
expect(controller.getDelay()).toBeLessThan(33000)
37+
controller.countError()
38+
expect(controller.getDelay()).toBeGreaterThanOrEqual(64000)
39+
expect(controller.getDelay()).toBeLessThan(65000)
40+
controller.countError()
41+
expect(controller.getDelay()).toBeGreaterThanOrEqual(128000)
42+
expect(controller.getDelay()).toBeLessThan(129000)
43+
controller.countError()
44+
expect(controller.getDelay()).toBeGreaterThanOrEqual(256000)
45+
expect(controller.getDelay()).toBeLessThan(257000)
46+
controller.countError()
47+
expect(controller.getDelay()).toBeGreaterThanOrEqual(512000)
48+
expect(controller.getDelay()).toBeLessThan(513000)
4349
// Maximum reached - additional errors should not increase the delay further
4450
controller.countError()
45-
expect(controller.getDelay()).toBeGreaterThanOrEqual(32000)
46-
expect(controller.getDelay()).toBeLessThan(33000)
51+
expect(controller.getDelay()).toBeGreaterThanOrEqual(512000)
52+
expect(controller.getDelay()).toBeLessThan(513000)
4753
})
4854

4955
it('resets the error count when reset is called', () => {

‎packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,7 @@ describe('httpPollingDatafileManager', () => {
555555
manager = createTestManager({
556556
sdkKey: '456',
557557
updateInterval: 1000,
558-
urlTemplate: 'https://localhost:5556/datafiles/$SDK_KEY',
558+
urlTemplate: 'https://localhost:5556/datafiles/%s',
559559
})
560560
})
561561

@@ -572,4 +572,25 @@ describe('httpPollingDatafileManager', () => {
572572
await manager.onReady()
573573
})
574574
})
575+
576+
describe('when constructed with an update interval below the minimum', () => {
577+
beforeEach(() => {
578+
manager = createTestManager({ sdkKey: '123', updateInterval: 500, autoUpdate: true })
579+
})
580+
581+
it('uses the default update interval', async () => {
582+
manager.queuedResponses.push({
583+
statusCode: 200,
584+
body: '{"foo3": "bar3"}',
585+
headers: {},
586+
})
587+
588+
const setTimeoutSpy: jest.SpyInstance<() => void, [() => void, number]> = jest.spyOn(testTimeoutFactory, 'setTimeout')
589+
590+
manager.start()
591+
await manager.onReady()
592+
expect(setTimeoutSpy).toBeCalledTimes(1)
593+
expect(setTimeoutSpy.mock.calls[0][1]).toBe(300000)
594+
})
595+
})
575596
})

‎packages/datafile-manager/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@
3535
"typescript": "^3.3.3333"
3636
},
3737
"dependencies": {
38-
"@optimizely/js-sdk-logging": "^0.1.0"
38+
"@optimizely/js-sdk-logging": "^0.1.0",
39+
"@optimizely/js-sdk-utils": "^0.1.0"
3940
},
4041
"scripts": {
4142
"test": "jest",

‎packages/datafile-manager/src/backoffController.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,7 @@
1414
* limitations under the License.
1515
*/
1616

17-
import {
18-
BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT,
19-
BACKOFF_MAX_ERROR_COUNT,
20-
} from './config'
17+
import { BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT } from './config'
2118

2219
function randomMilliseconds() {
2320
return Math.round(Math.random() * 1000)
@@ -38,7 +35,7 @@ export default class BackoffController {
3835
}
3936

4037
countError(): void {
41-
if (this.errorCount < BACKOFF_MAX_ERROR_COUNT) {
38+
if (this.errorCount < BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT.length - 1) {
4239
this.errorCount++
4340
}
4441
}

‎packages/datafile-manager/src/config.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,6 @@ export const DEFAULT_UPDATE_INTERVAL = 5 * 60 * 1000 // 5 minutes
1818

1919
export const MIN_UPDATE_INTERVAL = 1000
2020

21-
export const SDK_KEY_TOKEN = '$SDK_KEY'
21+
export const DEFAULT_URL_TEMPLATE = `https://cdn.optimizely.com/datafiles/%s.json`
2222

23-
export const DEFAULT_URL_TEMPLATE = `https://cdn.optimizely.com/datafiles/${SDK_KEY_TOKEN}.json`
24-
25-
export const BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT = [0, 2, 4, 8, 16, 32]
26-
27-
export const BACKOFF_MAX_ERROR_COUNT = 10
23+
export const BACKOFF_BASE_WAIT_SECONDS_BY_ERROR_COUNT = [0, 8, 16, 32, 64, 128, 256, 512]

‎packages/datafile-manager/src/httpPollingDatafileManager.ts

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,11 @@
1515
*/
1616

1717
import { getLogger } from '@optimizely/js-sdk-logging'
18+
import { sprintf } from '@optimizely/js-sdk-utils';
1819
import { DatafileManager, DatafileManagerConfig, DatafileUpdate } from './datafileManager';
1920
import EventEmitter from './eventEmitter'
2021
import { AbortableRequest, Response, Headers } from './http';
21-
import { DEFAULT_UPDATE_INTERVAL, MIN_UPDATE_INTERVAL, DEFAULT_URL_TEMPLATE, SDK_KEY_TOKEN } from './config'
22+
import { DEFAULT_UPDATE_INTERVAL, MIN_UPDATE_INTERVAL, DEFAULT_URL_TEMPLATE } from './config'
2223
import { TimeoutFactory, DEFAULT_TIMEOUT_FACTORY } from './timeoutFactory'
2324
import BackoffController from './backoffController';
2425

@@ -46,8 +47,6 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
4647

4748
private currentDatafile: object | null
4849

49-
private readonly sdkKey: string
50-
5150
private readonly readyPromise: Promise<void>
5251

5352
private isReadyPromiseSettled: boolean
@@ -68,7 +67,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
6867

6968
private lastResponseLastModified?: string
7069

71-
private urlTemplate: string
70+
private datafileUrl: string
7271

7372
private timeoutFactory: TimeoutFactory
7473

@@ -90,8 +89,6 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
9089
urlTemplate = DEFAULT_URL_TEMPLATE,
9190
} = configWithDefaultsApplied
9291

93-
this.sdkKey = sdkKey
94-
9592
this.isReadyPromiseSettled = false
9693
this.readyPromiseResolver = () => {}
9794
this.readyPromiseRejecter = () => {}
@@ -100,7 +97,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
10097
this.readyPromiseRejecter = reject
10198
})
10299

103-
if (typeof datafile !== 'undefined') {
100+
if (datafile) {
104101
this.currentDatafile = datafile
105102
this.resolveReadyPromise()
106103
} else {
@@ -109,10 +106,7 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
109106

110107
this.isStarted = false
111108

112-
this.urlTemplate = urlTemplate
113-
if (this.urlTemplate.indexOf(SDK_KEY_TOKEN) === -1) {
114-
logger.debug(`urlTemplate does not contain replacement token ${SDK_KEY_TOKEN}`)
115-
}
109+
this.datafileUrl = sprintf(urlTemplate, sdkKey)
116110

117111
this.timeoutFactory = timeoutFactory
118112
this.emitter = new EventEmitter()
@@ -165,10 +159,6 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
165159
return this.emitter.on(eventName, listener)
166160
}
167161

168-
private getUrl(sdkKey: string) {
169-
return this.urlTemplate.replace(SDK_KEY_TOKEN, sdkKey)
170-
}
171-
172162
private onRequestRejected(err: any): void {
173163
if (!this.isStarted) {
174164
return
@@ -236,10 +226,8 @@ export default abstract class HTTPPollingDatafileManager implements DatafileMana
236226
headers['if-modified-since'] = this.lastResponseLastModified
237227
}
238228

239-
const datafileUrl = this.getUrl(this.sdkKey)
240-
241-
logger.debug('Making datafile request to url %s with headers: %s', datafileUrl, () => JSON.stringify(headers))
242-
this.currentRequest = this.makeGetRequest(datafileUrl, headers)
229+
logger.debug('Making datafile request to url %s with headers: %s', this.datafileUrl, () => JSON.stringify(headers))
230+
this.currentRequest = this.makeGetRequest(this.datafileUrl, headers)
243231

244232
const onRequestComplete = () => {
245233
this.onRequestComplete()

0 commit comments

Comments
 (0)