STITCH-2584 Java SDK: Use bulk operations wherever possible on local database for sync #115
Conversation
android/build.gradle
Outdated
| } | ||
| testInstrumentationRunnerArgument "test.stitch.baseURL", properties.getProperty("test.stitch.baseURL", "http://10.0.2.2:9090") | ||
| testInstrumentationRunnerArgument "test.stitch.baseURL", | ||
| properties.getProperty("test.stitch.baseURL", "http://10.1.14.181:9090") |
| implementation "com.android.support:appcompat-v7:${support_library_version}" | ||
| implementation "com.android.support:recyclerview-v7:${support_library_version}" | ||
| implementation "com.android.support.constraint:constraint-layout:1.1.0" | ||
| // implementation 'org.jetbrains.kotlinx:kotlinx-coroutines-core:1.1.1' |
There was a problem hiding this comment.
Can just delete, we don't need core.
| <manifest package="com.mongodb.stitch.android.services.mongodb.remote"> | ||
| <manifest package="com.mongodb.stitch.android.services.mongodb.remote" | ||
| xmlns:android="http://schemas.android.com/apk/res/android"> | ||
| <application android:usesCleartextTraffic="true" /> |
There was a problem hiding this comment.
I think you can undo these changes
|
|
||
| override fun getStitchBaseURL(): String { | ||
| return InstrumentationRegistry.getArguments().getString("test.stitch.baseURL", "http://10.0.2.2:9090") | ||
| return "http://10.0.2.2:9090" |
| ) { | ||
| Map<BsonValue, CoreDocumentSynchronizationConfig> configs = new HashMap<>(); | ||
| for (final BsonValue documentId : documentIds) { | ||
| if (getSynchronizedDocument(documentId) == null) { |
There was a problem hiding this comment.
[q] Should we add a getSynchronizedDocuments to speed this up?
There was a problem hiding this comment.
I don't think we need to, it's just reading from a hashmap.
| * documents. | ||
| */ | ||
| public class DataSynchronizer implements NetworkMonitor.StateListener { | ||
| class BatchOps { |
There was a problem hiding this comment.
[nit] May be better to move this into its own class in a different file instead of this being an inner non-static class. Would just need to provide more args for its methods though to compensate not having direct access. Might be cleaner though.
| } | ||
|
|
||
| void merge(@Nullable final BatchOps batchOps) { | ||
| if (batchOps != null) { |
There was a problem hiding this comment.
[nit] Can make this if null early return to reduce indentation
| undoCollection.deleteOne(getDocumentIdFilter(documentId)); | ||
| eventsToEmit.add(event); | ||
| } | ||
| triggerListeningToNamespace(namespace); |
There was a problem hiding this comment.
[q] Do we need to trigger here as a part of this change?
There was a problem hiding this comment.
Yes, desync used to trigger every time it was called. It no longer does that, so we need to do it manually after desyncing has been done. There is a cleaner way to do this by tracking what operations have been done and if the streams need to be restarted.
| } | ||
| } | ||
|
|
||
| void setPendingWritesCompleteNoDB(final BsonDocument atVersion) { |
There was a problem hiding this comment.
[q] Is the point of this to make sync not skip over these during batch processing?
There was a problem hiding this comment.
The point of this is to not replace the doc config everytime we set pending writes– that's very slow. So we change it in memory here and do a bulk write after the pass on the namespace is done.
| new BsonDocument( | ||
| CoreDocumentSynchronizationConfig.ConfigCodec.Fields.IS_STALE, | ||
| new BsonBoolean(stale)))); | ||
| // docsColl.updateOne( |
There was a problem hiding this comment.
Should this be uncommented or method removed?
| new BsonDocument( | ||
| CoreDocumentSynchronizationConfig.ConfigCodec.Fields.IS_STALE, | ||
| new BsonBoolean(stale)))); | ||
| isStale = stale; |
There was a problem hiding this comment.
[q] approve of caching locally but looks like this never gets written to the docConfig as a result - are we ok with this?
There was a problem hiding this comment.
It does get written when we do the bulk writes.
In the event that the app crashes before it can be written, the document is marked stale anyway on app restart.
| * @param atVersion the version for which the write occurred. | ||
| * @param changeEvent the description of the write/change. | ||
| */ | ||
| void setSomePendingWrites( |
There was a problem hiding this comment.
[q] why add this? does not appear this is called anywhere? did I miss it?
No description provided.