-
Notifications
You must be signed in to change notification settings - Fork 85
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: Implement shedding identity queue #343
Conversation
This pull request has been linked to Shortcut Story #233103: Add timeout to identify handler. |
2a4ac75
to
84fdb36
Compare
@@ -278,15 +278,53 @@ public class LDClient { | |||
- parameter context: The LDContext set with the desired context. | |||
- parameter completion: Closure called when the embedded `setOnlineIdentify` call completes, subject to throttling delays. (Optional) | |||
*/ | |||
@available(*, deprecated, message: "Use LDClient.identify(context: completion:) with non-optional completion parameter") |
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.
Deprecating this method so that customers will start moving over to the callback that informs them of success or failure.
- parameter context: The LDContext set with the desired context. | ||
- parameter completion: Closure called when the embedded `setOnlineIdentify` call completes, subject to throttling delays. (Optional) | ||
*/ | ||
public func identify(context: LDContext, completion: @escaping (_ result: IdentifyResult) -> Void) { |
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.
The other identify method has an optional completion method. This one isn't optional.
If I continue with the completion method being optional, then the method isn't fully disambiguated. Customers could have code like: LDClient.get()?.identify(context: context, completion: nil)
and the compiler wouldn't know which one we meant.
Given that these identify requests might never actually complete, it seems kind of reasonable to force customers listen for those results, just as a measure of good practice for how to use this SDK. What do you think?
If we decide to make it optional, how do we deal with the backwards compatibility naming convention? Keep in mind that if we name it something like identifyWithShedding
then in the next major release, customers who took action based on the deprecation notice are going to have to change their code AGAIN.
We could consider making it non-nullable in this. version and loosen that restriction in the next. Or we could wait to see what customer feedback is and add the nullability back at that time.
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.
I am interested to see what customer feedback is. It doesn't seem unreasonable at this point to require it.
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.
They can always pass a no-op and ignore it.
84fdb36
to
8c77ce4
Compare
Previously, customers could queue a boundless limit of identify requests. The SDK would work its way through this FIFO queue, processing all intermediate but unnecessary requests. With this change, intermediate identify requests will be shed from the processing queue. NOTE: To preserve backwards compatibility, the original identify method will queue up "unsheddable" tasks which will continue to queue as before. Usage of the new `identify` method will allow developers to opt-in to this new behavior.
8c77ce4
to
9054985
Compare
Can/should the contract tests be updated to use the shedding version? |
🤖 I have created a release *beep* *boop* --- ## [9.4.0](9.3.0...9.4.0) (2024-02-21) ### Features * Add new identify method with time out support ([#344](#344)) ([34ba8ab](34ba8ab)) * Implement shedding identity queue ([#343](#343)) ([393a28c](393a28c)) * Introduce variation method with generic return types ([#342](#342)) ([7ff2ffb](7ff2ffb)) ### Bug Fixes * Add privacy manifest ([#334](#334)) ([154fde7](154fde7)) * Ensure anonymous context is valid ([#338](#338)) ([65406cc](65406cc)) * Replace simple logger with os_log statements ([#340](#340)) ([7ba4397](7ba4397)) --- 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>
Previously, customers could queue a boundless limit of identify requests. The SDK would work its way through this FIFO queue, processing all intermediate but unnecessary requests.
With this change, intermediate identify requests will be shed from the processing queue.
NOTE: To preserve backwards compatibility, the original identify method will queue up "unsheddable" tasks which will continue to queue as before. Usage of the new
identify
method will allow developers to opt-in to this new behavior.