feat: Honor polling interval between restarts #355
Conversation
|
This pull request has been linked to Shortcut Story #235800: Honor polling interval between sync. restarts. |
keelerm84
force-pushed
the
mk/sc-235800/polling-delay
branch
3 times, most recently
from
b4dabac
to
afb32ad
Compare
| guard let lastUpdated = cachedContexts[cacheKey] | ||
| else { return (items: cachedFlags.flags, etag: etag, lastUpdated: nil) } | ||
|
|
||
| return (items: cachedFlags.flags, etag: etag, lastUpdated: Date(timeIntervalSince1970: TimeInterval(lastUpdated / 1_000))) |
There was a problem hiding this comment.
Just to confirm here, the lastUpdate from the cache is millis, but the TimeInterval takes in seconds as a float?
There was a problem hiding this comment.
Yes.
A TimeInterval is always in seconds.
You can see here that the timestamp in the cache is millis.
| @@ -5,7 +5,7 @@ protocol FeatureFlagCaching { | |||
| // sourcery: defaultMockValue = KeyedValueCachingMock() | |||
| var keyedValueCache: KeyedValueCaching { get } | |||
|
|
|||
| func getCachedData(cacheKey: String) -> (items: StoredItems?, etag: String?) | |||
| func getCachedData(cacheKey: String) -> (items: StoredItems?, etag: String?, lastUpdated: Date?) | |||
There was a problem hiding this comment.
Missing documentation. The other method also is missing documentation around lastUpdated.
There was a problem hiding this comment.
These methods are internal and so won't be reflected in the public documentation. I have still added some additional documentation.
| } else { | ||
| flagRequestTimer = LDTimer(withTimeInterval: pollingInterval, fireQueue: syncQueue, execute: processTimer) | ||
| makeFlagRequest(isOnline: true) | ||
| } |
There was a problem hiding this comment.
It seems like it should be possible to make these two code paths converge. If the fireAt is in the past, does the LDTimer fire immediately?
If so, seems like defaulting last cached requested time to epoch when it is missing should work.
Perhaps I'm missing why makeFlagRequest(...) is in one path but not the other and that reason I'm missing is why the paths can't converge?
There was a problem hiding this comment.
It was affecting unit test execution order. I've updated the unit tests and combined these two code paths.
If an application receives a flag update and then is closed, only to be opened back up, we shouldn't need to make another polling request immediately as the data is within the acceptable freshness threshold. The same is true when the app is put in the background and brought forward again.
keelerm84
force-pushed
the
mk/sc-235800/polling-delay
branch
from
afb32ad
to
82f8192
Compare
🤖 I have created a release *beep* *boop* --- ## [9.6.0](9.5.1...9.6.0) (2024-03-20) ### Features * Honor polling interval between restarts ([#355](#355)) ([bd58864](bd58864)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: LaunchDarklyReleaseBot <LaunchDarklyReleaseBot@launchdarkly.com>
If an application receives a flag update and then is closed, only to be opened back up, we shouldn't need to make another polling request immediately as the data is within the acceptable freshness threshold.
The same is true when the app is put in the background and brought forward again.