Skip to content
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.

Sign up for GitHub

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

fix: evaluation scheduler not being reset when the request succeeds #37

Merged
merged 2 commits into from
Aug 31, 2023
Merged

fix: evaluation scheduler not being reset when the request succeeds #37

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fff440b
fix: evaluation scheduler not being reset when the request succeeds
cre8ivejp Aug 30, 2023
0ad7ed9
test: fix e2e test
cre8ivejp Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions Bucketeer/Sources/Internal/Scheduler/EvaluationForegroundTask.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Foundation

final class EvaluationForegroundTask: ScheduledTask {
private weak var component: Component?
private let component: Component
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component at this point can't be nil.

private let queue: DispatchQueue
private var poller: Poller?
private var retryPollingInterval: Int64
Expand All @@ -20,11 +20,10 @@ final class EvaluationForegroundTask: ScheduledTask {
self.maxRetryCount = maxRetryCount
}

private func reschedule(isRetrying: Bool) {
private func reschedule(interval: Int64) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored this function to keep the consistency with Android SDK.
Reference: https://github.com/bucketeer-io/android-client-sdk/blob/main/bucketeer/src/main/kotlin/io/bucketeer/sdk/android/internal/scheduler/EvaluationForegroundTask.kt#L23-L31

self.stop()
guard let component = component else { return }
self.poller = .init(
intervalMillis: isRetrying ? retryPollingInterval : component.config.pollingInterval,
intervalMillis: interval,
queue: queue,
logger: component.config.logger,
handler: { [weak self] _ in
Expand All @@ -37,7 +36,7 @@ final class EvaluationForegroundTask: ScheduledTask {
}

func start() {
reschedule(isRetrying: false)
reschedule(interval: self.component.config.pollingInterval)
}

func stop() {
Expand All @@ -46,7 +45,6 @@ final class EvaluationForegroundTask: ScheduledTask {
}

private func fetchEvaluations() {
guard let component = component else { return }
let eventInteractor = component.eventInteractor
let retryCount = self.retryCount
let maxRetryCount = self.maxRetryCount
Expand All @@ -61,8 +59,11 @@ final class EvaluationForegroundTask: ScheduledTask {
seconds: response.seconds,
sizeByte: response.sizeByte
)
// reset retry count
self?.retryCount = 0
// reset the scheduler to use the default polling interval configured in the BKTConfig
if (retryCount > 0) {
self?.retryCount = 0
self?.reschedule(interval: pollingInterval)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the polling interval setting not being reset when the request succeeds after retrying.

}

case .failure(let error, let featureTag):
try eventInteractor.trackFetchEvaluationsFailure(
Expand All @@ -79,16 +80,19 @@ final class EvaluationForegroundTask: ScheduledTask {
// we can retry more
self?.retryCount += 1
if !retried {
self?.reschedule(isRetrying: true)
// we reschedule just once and wait until it reaches
// the max retrying count or succeeds to reschedule it again
// to use the default polling interval configured in the BKTConfig
self?.reschedule(interval: retryPollingInterval)
}
} else {
// we already retried enough, let's get back to daily job
self?.retryCount = 0
self?.reschedule(isRetrying: false)
self?.reschedule(interval: pollingInterval)
}
}
} catch let error {
self?.component?.config.logger?.error(error)
self?.component.config.logger?.error(error)
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions BucketeerTests/EvaluationForegroundTaskTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import XCTest
final class EvaluationForegroundTaskTests: XCTestCase {
func testStartAndReceiveSuccess() {
let expectation = self.expectation(description: "")
expectation.expectedFulfillmentCount = 3
expectation.expectedFulfillmentCount = 2
expectation.assertForOverFulfill = true
let dispatchQueue = DispatchQueue(label: "default", qos: .default)

Expand Down Expand Up @@ -93,7 +93,7 @@ final class EvaluationForegroundTaskTests: XCTestCase {
let task = EvaluationForegroundTask(
component: component,
queue: dispatchQueue,
retryPollingInterval: 1,
retryPollingInterval: 1000,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 millisecond is too low and the test could fail sometimes, so I'm increasing it by 1 second.

maxRetryCount: 5
)
task.start()
Expand Down