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

Jump to bottom

Read-Your-Write Consistency #2168

Merged
merged 12 commits into from
Sep 30, 2024
Merged

Read-Your-Write Consistency #2168

merged 12 commits into from
Sep 30, 2024

Conversation

rgomezp
Copy link
Contributor

@rgomezp rgomezp commented Aug 6, 2024 •
edited
Loading

Description

One Line Summary

Introduce and integrate a ConsistencyManager to manage read-your-write tokens for improved segment membership calculation.

Details

Motivation

This update introduces the ConsistencyManager to manage kafka offsets, which are used as read-your-write tokens. The goal is to improve the accuracy of segment membership calculations by providing an open-ended & highly customizable blocking mechanism for operations that rely on having successfully synchronized client & server state.

For a first use-case, we want to block the fetching of IAMs until we have offsets for a user & subscription state update.

Scope

  • Registers the ConsistencyManager in the User and InAppMessages modules.
  • Updates the Subscription and User backend services to return offsets needed by the ConsistencyManager.
  • Modifies Subscription and UpdateUser operation executors to set offsets in the ConsistencyManager.
  • Updates the IAM manager and backend service to include the offset as a query parameter in the listInAppMessages request.

Testing

Unit testing

  • New coverage for new ConsistencyManager class

Manual testing

Tested on Android emulator

  • Set up Android test app in OneSignal
  • Note the current app version (e.g. 1.0)
  • Create a segment targeting a specific version (e.g. 1.1)
  • Create an in-app using the new segment
  • Open the app (see you don't get an in-app)
  • Close the app
  • Change the version to the one in the segment (e.g. 1.1)
  • Open the app (see you get the in-app)

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@rgomezp rgomezp force-pushed the read-your-write branch 15 times, most recently from ecaef33 to 4ecae20 Compare August 9, 2024 22:03
@rgomezp rgomezp changed the title Read-Your-Write Consistency via KafkaOffsetManager Read-Your-Write Consistency Aug 9, 2024
@rgomezp rgomezp force-pushed the read-your-write branch 3 times, most recently from f54bc44 to 1bf9985 Compare August 14, 2024 19:39
rgomezp
rgomezp commented Aug 19, 2024
View reviewed changes
...K/onesignal/in-app-messages/src/main/java/com/onesignal/inAppMessages/InAppMessagesModule.kt Outdated Show resolved Hide resolved
...lSDK/onesignal/core/src/main/java/com/onesignal/common/consistency/IamFetchReadyCondition.kt Outdated Show resolved Hide resolved
...al/core/src/main/java/com/onesignal/user/internal/backend/impl/SubscriptionBackendService.kt Show resolved Hide resolved
...al/core/src/main/java/com/onesignal/user/internal/backend/impl/SubscriptionBackendService.kt Show resolved Hide resolved
.../onesignal/core/src/main/java/com/onesignal/user/internal/backend/impl/UserBackendService.kt Show resolved Hide resolved
.../java/com/onesignal/user/internal/operations/impl/executors/SubscriptionOperationExecutor.kt Outdated Show resolved Hide resolved
...in/java/com/onesignal/user/internal/operations/impl/executors/UpdateUserOperationExecutor.kt Outdated Show resolved Hide resolved
...in/java/com/onesignal/user/internal/operations/impl/executors/UpdateUserOperationExecutor.kt Outdated Show resolved Hide resolved
@rgomezp rgomezp marked this pull request as ready for review August 19, 2024 23:08
@rgomezp rgomezp requested a review from jkasten2 August 19, 2024 23:42
@rgomezp rgomezp force-pushed the read-your-write branch 2 times, most recently from f5a6a2b to 51aaa8b Compare September 24, 2024 21:53
@rgomezp rgomezp requested a review from Koji23 September 24, 2024 21:57
@rgomezp rgomezp force-pushed the read-your-write branch 2 times, most recently from 601ec15 to bc9963b Compare September 25, 2024 17:56
Koji23
Koji23 reviewed Sep 25, 2024
View reviewed changes
Copy link

@Koji23 Koji23 left a comment

Choose a reason for hiding this comment

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

this LGTM from a non-iOS expert view

@rgomezp
ConsistencyManager interface & implementation
6e05278 
Manages read-your-write tokens. The manager works based on conditions & tokens.

Tokens are stored in a nested map indexed by a unique id (e.g. `onesignalId`) and a token key (e.g. `USER`).

This allows us to track tokens on a per-user basis (e.g. handle switching users).

Conditions work by creating a blocking mechanism with customizable token retrieval until a pre-defined condition is met (e.g. at least two specific tokens are available).

Also allows extensibility for future applications to control offset blocking mechanism in consistency use-cases.
@rgomezp
DI: register ConsistencyManager in UserModule
ca6fe5a 
Inject ConsistencyManager where it will be used.
@rgomezp
IamFetchReadyCondition Implementation
32775a0 
Motivation: custom condition to block token retrieval until condition is met.

We then return the newest token
@rgomezp
Add offset, retryCount, & sessionDuration to OptionalHeaders
1505d07 
Motivation: support passing values as headers, add comments
@rgomezp
Update Subscription & User backend services to return offsets
be10255 
Motivation: need the offsets to be available to set in the ConsistencyManager
@rgomezp
Update IAM manager & backend service with retry logic, optional headers
1a43567 
Motivation: the IAM fetch call (`listInAppMessages`) will include the rywToken, retryCount, & secondsSinceAppOpen (tracked on backend)

We update the request & related code here.

Handle retry logic
@rgomezp
Flush track session start operation
591bbe7 
Motivation: we want to update the user as soon as possible in order to not delay IAM fetch
@rgomezp
IamFetchRywTokenKey Enum
c3f15f5 
Motivation: to be used to index read your write tokens specific to the IamFetch consistency use case
@rgomezp
Update User & Subscription operation executors to set the tokens
5b83545 
Motivation: the executors call the respective backend services who's result will include the token value. We then hold in memory via `setRywToken`
@rgomezp
Update HttpClient to put new optional headers into request
2083bd0 
Motivation: offset, secondsSinceAppOpen, & retryCount will be sent as headers
jkasten2
jkasten2 requested changes Sep 26, 2024
View reviewed changes
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Left a few more comments in the code.

Also a few more things not in the code:

  • Manual testing still says "// TO DO"
  • Some other things not checked off in the PR check list.
  • Does this code work with that is available on the backend today? If not we will need to either wait until the backend is shipped until we merge this PR into main, or make changes to this PR to support that.

...lSDK/onesignal/core/src/main/java/com/onesignal/common/consistency/IamFetchReadyCondition.kt Outdated Show resolved Hide resolved
...sages/src/main/java/com/onesignal/inAppMessages/internal/backend/impl/InAppBackendService.kt Outdated Show resolved Hide resolved
...sages/src/main/java/com/onesignal/inAppMessages/internal/backend/impl/InAppBackendService.kt Outdated Show resolved Hide resolved
...sages/src/main/java/com/onesignal/inAppMessages/internal/backend/impl/InAppBackendService.kt Outdated Show resolved Hide resolved
@rgomezp
Copy link
Contributor Author

rgomezp commented Sep 27, 2024

  • Does this code work with that is available on the backend today? If not we will need to either wait until the backend is shipped until we merge this PR into main, or make changes to this PR to support that.

@jkasten2 yes. backend changes have been shipped

@rgomezp rgomezp merged commit 84d0d60 into main Sep 30, 2024
2 checks passed
@rgomezp rgomezp deleted the read-your-write branch September 30, 2024 17:54
@rgomezp rgomezp mentioned this pull request Sep 30, 2024
Merged
This was referenced Oct 2, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Koji23 Koji23 Koji23 left review comments

@jkasten2 jkasten2 jkasten2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rgomezp @jkasten2 @Koji23