-
Notifications
You must be signed in to change notification settings - Fork 283
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
Resource consolidation after using the AllChangesSquashedBundlePost upload strategy for resource creation. #2509
Conversation
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.
Please look into the meta tag overwriting issue, as we have encountered this problem.
engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/sync/upload/ResourceConsolidator.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/sync/upload/ResourceConsolidator.kt
Outdated
Show resolved
Hide resolved
...src/androidTest/java/com/google/android/fhir/sync/upload/HttpPostResourceConsolidatorTest.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt
Outdated
Show resolved
Hide resolved
...src/androidTest/java/com/google/android/fhir/sync/upload/HttpPostResourceConsolidatorTest.kt
Outdated
Show resolved
Hide resolved
...src/androidTest/java/com/google/android/fhir/sync/upload/HttpPostResourceConsolidatorTest.kt
Show resolved
Hide resolved
...src/androidTest/java/com/google/android/fhir/sync/upload/HttpPostResourceConsolidatorTest.kt
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt
Outdated
Show resolved
Hide resolved
...src/androidTest/java/com/google/android/fhir/sync/upload/HttpPostResourceConsolidatorTest.kt
Outdated
Show resolved
Hide resolved
...src/androidTest/java/com/google/android/fhir/sync/upload/HttpPostResourceConsolidatorTest.kt
Outdated
Show resolved
Hide resolved
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.
Thanks @santosh-pingle - just left some more comments.
Perhaps the most critical comment is that the new db function needs to be tested.
@FikriMilano also fyi. Thanks!
.../src/test/java/com/google/android/fhir/sync/upload/request/TransactionBundleGeneratorTest.kt
Outdated
Show resolved
Hide resolved
.../com/google/android/fhir/sync/upload/request/BundleEntryComponentGeneratorImplementations.kt
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/dao/ResourceDao.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/sync/upload/ResourceConsolidator.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt
Outdated
Show resolved
Hide resolved
...src/androidTest/java/com/google/android/fhir/sync/upload/HttpPostResourceConsolidatorTest.kt
Outdated
Show resolved
Hide resolved
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 not sure if this is already discussed, but here is use case that will need to be supported. We should discuss if there are any race conditions and potential OOMs -
CHW is offline for 2 weeks
Visits 30 houses per day
Each house has 4 family members
Each registration results in 30 resources per family member
This will create a total of 2x7x30x4x30 = 50,400 resources with references. Let us assume that there are no resources with base64 binary data. Each resource will roughly be 5KB that's roughly 100 MB of data to upload. 3G upload speed is around 0.4 MB/s. So this should take roughly 5 minutes to upload + post sync processing time. We could have some documentation around the performance of using POST vs PUT and the repercussions based on this.
I am looking into the review comments. |
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2499
Description
The resource is created locally, and the AllChangesSquashedBundlePost is used to synchronize the resource with the FHIR server. In return, the server sends a response containing the resource metadata and the newly assigned resource ID as BundleEntryComponent in the Bundle. Subsequently, the local database is updated with the provided resource.
Upload strategy : AllChangesSquashedBundlePost
Discard the local changes from LocalChangeEntity table.
Update the resource with the received resourceId and meta data in the local database.
Update the resource with the received reference id in the local database.
Note: This change does not handle resource consolidation if multiple bundles are sent in the upload request to create resources at the server.
Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Choose one: Feature
Screenshots (if applicable)
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.