feat(DataStore): Start/Stop implementation

[wooj2]

• Implementing start/stop was more difficult than expected due to supporting an offline use case.
  I am leaving the PR in draft for historical purposes. This approach attempts to introduce a state into the AWSDataStorePlugin to keep track of whether or not we've issued a start() to the sync engine. Doing so, in my opinion, overcomplicates the design.
  In addition, it increases overhead by starting a mostly no-op thread for each data store operation.

A couple of test cases to consider:

1. No API Configured, customer does not call start(), but publisher needs to be setup
2. API is configured, start(), query(), stop(), start() -- explicit case
3. API is configured, start(), query(), clear(), start() -- explicit case
4. API is configured, query(), query(), stop(), query() -- implicit case, where the first query() will start the sync engine

Known issues:
There is a known issue with this implementation. If customers attempt to call stop, and then immediately call clear(), the local database will not be cleared because the instance to the storage engine is set to nil. I don't think this is a use case we really have to worry about, so I am holding off on implementing this.

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

[codecov]

Codecov Report

Merging #917 (d2ec336) into main (ff776df) will decrease coverage by 0.07%.
The diff coverage is 32.46%.

@@            Coverage Diff             @@
##             main     #917      +/-   ##
==========================================
- Coverage   67.27%   67.19%   -0.08%     
==========================================
  Files         895      895              
  Lines       35372    35434      +62     
==========================================
+ Hits        23796    23810      +14     
- Misses      11576    11624      +48     

Flag	Coverage Δ
API_plugin_unit_test	62.49% <ø> (-0.08%)	⬇️
Analytics_plugin_unit_test	72.38% <ø> (ø)
Auth_plugin_unit_test	48.62% <ø> (ø)
DataStore_plugin_unit_test	82.73% <36.23%> (-0.30%)	⬇️
Predictions_plugin_unit_test	49.49% <ø> (ø)
Storage_plugin_unit_test	74.74% <ø> (ø)
build_test_amplify	63.08% <0.00%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...egories/DataStore/DataStoreCategory+Behavior.swift	12.50% <0.00%> (-4.17%)	⬇️
...nSync/Support/MockSQLiteStorageEngineAdapter.swift	50.46% <0.00%> (-2.42%)	⬇️
...TestCommon/Mocks/MockDataStoreCategoryPlugin.swift	32.35% <0.00%> (-4.32%)	⬇️
...ataStoreCategoryPlugin/Storage/StorageEngine.swift	48.96% <25.00%> (-0.59%)	⬇️
...gin/AWSDataStorePlugin+DataStoreBaseBehavior.swift	58.10% <29.16%> (-5.61%)	⬇️
...Plugin/Storage/StorageEngine+SyncRequirement.swift	44.68% <43.47%> (-10.88%)	⬇️
...WSDataStoreCategoryPlugin/AWSDataStorePlugin.swift	39.13% <50.00%> (+0.75%)	⬆️
...Sync/RemoteSync/RemoteSyncAPIInvocationTests.swift	94.20% <100.00%> (+0.08%)	⬆️
...ryPluginTests/TestSupport/SyncEngineTestBase.swift	96.22% <100.00%> (+0.03%)	⬆️
...lugin/Operation/AWSAPIOperation+APIOperation.swift	78.37% <0.00%> (-5.41%)	⬇️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff776df...b271be3. Read the comment docs.

[wooj2]

Not particularly happy with this this because:

• If customers do not have API configured, and have logs set to info, they will repeatedly get a info message of "Unable to find suitable API plugin for syncEngine. syncEngine will not be started"
• Having to create a serial queue to kick off sync engine is not preferable, plus, this happens each time there is a save/delete/etc

To remedy this, it seems that the only way to get around this is to either expose the state of the sync engine somehow -- either through a published combine event, or a member variable.. or something like this.

In any case, I'm sending this review out a bit early in case anyone can think of a better way of implementing this that is thread safe.

[wooj2]

Not particularly happy with this this because:

• If customers do not have API configured, and have logs set to info, they will repeatedly get a info message of "Unable to find suitable API plugin for syncEngine. syncEngine will not be started"
• Having to create a serial queue to kick off sync engine is not preferable, plus, this happens each time there is a save/delete/etc

To remedy this, it seems that the only way to get around this is to either expose the state of the sync engine somehow -- either through a published combine event, or a member variable.. or something like this.

In any case, I'm sending this review out a bit early in case anyone can think of a better way of implementing this that is thread safe.

I took another look at the code. Previously, the code storageEngine being instantiated was synonymous with the syncEngine being started (yes, you read that right.. these two actions were tightly coupled). Since we are changing the behavior to not automatically start the syncEngine, there is not a good way to separate out instantiation with starting the sync engine without introducing some state.

Option 1 (what is currently implemented):
Have the storage engine (but could be the sync engine) keep track of the state of whether or not we have already started the sync engine.
Thoughts: Not ideal because we have to have a SerialQueue to on each and every save()/delete() etc. This creates a constant overhead for each call the customer makes.

Option 2:
Change the behavior of the AWSDataStorePlugin so that the storage engine is not instantiated on configure(). Only a call to query/start/etc will instantiate the storage engine and then start it.
Thoughts: - This breaks a number of our unit tests because the AWSDataStorePlugin is setup to accept a StorageEngine in it's init(). Making this change feels invasive (at least on the surface), but I suspect this is what we really ought to do, rather than keep track of the state of the sync engine. That being said, I've implemented it here: #919