amplify-android icon indicating copy to clipboard operation
amplify-android copied to clipboard

fix(datastore): memory issues, add disableSync and disableSubscription

Open joekiller opened this issue 3 years ago • 2 comments

  • [X] PR title and description conform to Pull Request guidelines.

Issue #, if available: Fixes #1869 Fixes #1849

Description of changes:

Fix memory management

  - mutation outbox will now allocate 200 slots by default   - mutation outbox loads only first 100 mutations plus any pending mutations related to first 100 loaded   - SubscriptionProcessor will now only hold a maximum of 100 subscription events with a max age of 10 minutes

add options to disable Sync or Subscriptions

Give a "producer only" time series database the sync and subscription systems can be cumbersome or unnecessary. Sometimes people just want to push data from the datastore and this gives more flexibility to the engine, makes for faster startup in these conditions, and reduced the need to hack around these features with items such as having the graphQL endpoint respond with empty lists in the sync or unauthorized for a subscription.

  - use datastore configuration disableSync(Model.class) and disableSubscription(Model.class) to allow toggling if model should sync or receive subscriptions   - https://docs.amplify.aws/lib/datastore/sync/q/platform/android/ should be updated to reflect new options for Syncing data.  

Fix Pending Mutation Engine Halting on Error

Currently the mutation engine will halt on any given error. This update tries to resume reading updates every 30 seconds. Configurable via datastore option outboxErrorRestartDelay.

Remove timeouts

There are various timeouts that are effectively CPU or network bound. Removing the timeouts makes the system more likely to work on a diversity of client.

  - remove restart delay for tests as they were CPU bound and caused predicable failure when a thread goes slow   - remove Orchestrator semaphore timeout   - remove ITEM_PROCESSING_TIMEOUT_MS from MutationProcessor

How did you test these changes? (Please add a line here how the changes were tested)

  • [X] Added Unit Tests

  • [X] Tested on frontend (Add Screenshots) image

  • [ ] Successful logs (Attach them to the PR)

Documentation update required?

  • [ ] No
  • [X] Yes (Please include a PR link for the documentation update)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

joekiller avatar Aug 07 '22 01:08 joekiller

Unit tests were verified to run successfully on several workstations using

 openjdk version "1.8.0_322"
 OpenJDK Runtime Environment (build 1.8.0_322-bre_2022_02_28_15_01-b00)
 OpenJDK 64-Bit Server VM (build 25.322-b00, mixed mode)
 openjdk 11.0.16 2022-07-19
 OpenJDK Runtime Environment Homebrew (build 11.0.16+0)
 OpenJDK 64-Bit Server VM Homebrew (build 11.0.16+0, mixed mode)
 openjdk version "1.8.0_345"
 OpenJDK Runtime Environment (build 1.8.0_345-b01)
 OpenJDK 64-Bit Server VM (build 25.345-b01, mixed mode)

joekiller avatar Aug 07 '22 02:08 joekiller

A little more testing is making me very happy with this solution. In my previous comment I had mentioned a theoretical maximum number of offline records of 27868 which on my stress testbed I have blown by and have recorded 172830 records, all while offline. That's a little over 6x the previous maximum. The testbed is creating approximately 167 records a minute. All memory consumption is very nominal.

joekiller avatar Aug 07 '22 15:08 joekiller

Hello @joekiller we are evaluating this PR. Can you please update it to resolve the conflicts?

mikepschneider avatar Dec 21 '22 19:12 mikepschneider

@joekiller The changes regarding disabling sync or subscriptions per type will introduce differences across platforms that will require more consideration but we like the changes regarding memory issues and timeouts. If you can split those into a separate PR we should be able to accept them.

mikepschneider avatar Dec 21 '22 20:12 mikepschneider

@mikepschneider I'll work to split it per your request.

joekiller avatar Dec 22 '22 14:12 joekiller

@mikepschneider I'm closing this for https://github.com/aws-amplify/amplify-android/pull/2217

joekiller avatar Jan 05 '23 02:01 joekiller