-
Notifications
You must be signed in to change notification settings - Fork 257
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
Restore Granular Upload Progress Updates and Database Consolidation #2392
Conversation
engine/src/main/java/com/google/android/fhir/sync/upload/request/UploadRequestGenerator.kt
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.
Excellent work @MJ1998 !! Have a few suggestions and added some comment related to the existing issues that I found in the code. The existing code issues are mostly harmless and can be fixed in later PR's by the team.
demo/src/main/java/com/google/android/fhir/demo/data/DemoFhirSyncWorker.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/impl/FhirEngineImpl.kt
Outdated
Show resolved
Hide resolved
engine/src/main/java/com/google/android/fhir/sync/upload/Uploader.kt
Outdated
Show resolved
Hide resolved
engine/src/test/java/com/google/android/fhir/sync/upload/UploaderTest.kt
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.
LGTM 👍
demo/src/main/java/com/google/android/fhir/demo/data/DemoFhirSyncWorker.kt
Show resolved
Hide resolved
…oogle#2392) * Upload giving intermediate progresses * emit uploadresults until its a Failure * tests * add test to verify multiple results emit * extra code * fix test issue * update merge * resolve comments
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2355 #2408
Description
syncUpload's upload lambda changed from
upload: (suspend (List<LocalChange>) -> UploadSyncResult)
TOupload: (suspend (List<LocalChange>) -> Flow<UploadRequestResult>)
Removed
UploadSyncResult
as its not needed anymore.Uploader.upload returns a Flow
Modified UploadRequestResult.Failure to have information about localChanges.
Added bundleSize as a config parameter to
UploadRequestGeneratorMode.BundleRequest
to test with bundleSize = 1Added test case to check flow of multiple results being returned.
Alternative(s) considered
Alternative was to solve for #2408 only by returning partial successes also in the consolidated
UploadSyncResult
in case there is a failure. This is developer-convenient as it does not require much change but does not solve both the issues. Also, we anticipate changing this API (in fact potentially moving this API completely) and minimal change could be preferred. However, with a little developer pain we can solve both the issues. Discussed with @aditya-07.Type
Choose one: (Bug fix | Feature | Documentation | Testing | Code health | Builds | Releases | Other)
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.