[Data sync] Rewrite data and photo sync workers to be more robust and readable #2867

gino-m · 2024-11-22T22:05:27Z

Try to process entire queue each time worker is run rather than just mutation for a specific enqueued LOI. This makes the sync worker more robust in the case that a worker failed for a particular LOI before it could request a retry() or update the mutation status.
Introduces the UploadQueueEntry wrapper / grouping for mutations to be used by the workers and UI.

Towards #2840

codecov · 2024-11-22T22:16:14Z

Codecov Report

Attention: Patch coverage is 84.26966% with 14 lines in your changes missing coverage. Please review.

Project coverage is 61.68%. Comparing base (c982a50) to head (a3cd4bf).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...droid/ground/persistence/sync/MediaUploadWorker.kt	73.68%	1 Missing and 4 partials ⚠️
...ogle/android/ground/ui/home/HomeScreenViewModel.kt	0.00%	2 Missing and 2 partials ⚠️
...le/android/ground/repository/MutationRepository.kt	93.02%	0 Missing and 3 partials ⚠️
.../android/ground/repository/SubmissionRepository.kt	0.00%	1 Missing ⚠️
.../main/java/com/google/android/ground/util/Debug.kt	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2867      +/-   ##
============================================
+ Coverage     61.19%   61.68%   +0.48%     
+ Complexity     1183     1173      -10     
============================================
  Files           267      268       +1     
  Lines          6396     6344      -52     
  Branches        914      876      -38     
============================================
- Hits           3914     3913       -1     
+ Misses         1943     1890      -53     
- Partials        539      541       +2

Files with missing lines	Coverage Δ
.../src/main/java/com/google/android/ground/Config.kt	`100.00% <ø> (ø)`
...ndroid/ground/model/mutation/SubmissionMutation.kt	`89.47% <ø> (+3.11%)`	⬆️
...ndroid/ground/model/submission/UploadQueueEntry.kt	`100.00% <100.00%> (ø)`
...ground/persistence/sync/LocalMutationSyncWorker.kt	`96.00% <100.00%> (+2.97%)`	⬆️
.../ground/repository/LocationOfInterestRepository.kt	`52.72% <100.00%> (ø)`
.../android/ground/repository/SubmissionRepository.kt	`18.60% <0.00%> (ø)`
.../main/java/com/google/android/ground/util/Debug.kt	`71.42% <0.00%> (-5.50%)`	⬇️
...le/android/ground/repository/MutationRepository.kt	`87.50% <93.02%> (+2.88%)`	⬆️
...ogle/android/ground/ui/home/HomeScreenViewModel.kt	`79.24% <0.00%> (-1.46%)`	⬇️
...droid/ground/persistence/sync/MediaUploadWorker.kt	`84.84% <73.68%> (+9.40%)`	⬆️

... and 11 files with indirect coverage changes

gino-m · 2024-11-27T20:26:01Z

@shobhitagarwal1612 @anandwana001 PTAL?

shobhitagarwal1612

I took a first pass and left some comments. Will take another pass soon.

ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorker.kt

shobhitagarwal1612 · 2024-11-28T04:48:40Z

ground/src/main/java/com/google/android/ground/util/Debug.kt

@@ -40,3 +42,5 @@ object Debug {
      null
    }
 }
+
+fun Throwable.priority() = if (this is FirebaseNetworkException) Log.DEBUG else Log.ERROR


How about adding a check here and log to console?

Then we can continue to use Timber.e normally in all classes

Those events shouldn't be shown as errors in local logs either, and I'm not sure we want to have the priority change magically without any indication in the code doing the logging.

shobhitagarwal1612 · 2024-11-28T04:51:36Z

ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorker.kt

    } catch (t: Throwable) {
-      Timber.e(t, "Failed to upload photo")
-      kotlin.Result.failure(t)
+      Timber.log(t.priority(), t, "Photo upload failed")


Please see the other comment regarding reverting this change

See response in other comment. Perhaps an extension function would be more elegant here? Timber.log override with no "priority", for example?

Forked this discussion into a suggestion in #2891.

ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt

ground/src/main/java/com/google/android/ground/model/submission/UploadQueueEntry.kt

…o gino-m/2840/improve-upload-queue-1

gino-m

@shobhitagarwal1612 PTAL at my comments/questions

ground/src/main/java/com/google/android/ground/model/submission/UploadQueueEntry.kt

ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt

gino-m · 2024-12-02T15:39:42Z

ground/src/main/java/com/google/android/ground/util/Debug.kt

@@ -40,3 +42,5 @@ object Debug {
      null
    }
 }
+
+fun Throwable.priority() = if (this is FirebaseNetworkException) Log.DEBUG else Log.ERROR


Those events shouldn't be shown as errors in local logs either, and I'm not sure we want to have the priority change magically without any indication in the code doing the logging.

gino-m · 2024-12-02T15:42:54Z

ground/src/main/java/com/google/android/ground/persistence/sync/MediaUploadWorker.kt

    } catch (t: Throwable) {
-      Timber.e(t, "Failed to upload photo")
-      kotlin.Result.failure(t)
+      Timber.log(t.priority(), t, "Photo upload failed")


See response in other comment. Perhaps an extension function would be more elegant here? Timber.log override with no "priority", for example?

…om/google/ground-android into gino-m/2840/improve-upload-queue-1

ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt

gino-m added 6 commits November 22, 2024 16:25

Add wrapper for related mutations

ea9ec4c

Repository accessor for upload queue

430b18e

Add ktdoc

7b95989

Update repo methods

98fc8b2

Update ktdoc

f4995d3

Update ktdoc

8c0193a

gino-m added 2 commits November 25, 2024 12:10

Rewrite sync worker WIP

7815e5b

Stop syncing per LOI

375a906

gino-m force-pushed the gino-m/2840/improve-upload-queue-1 branch from 3a0d9d4 to 375a906 Compare November 25, 2024 17:16

gino-m added 16 commits November 25, 2024 14:01

Simplify upload queue API

52d3d7d

Enqueue media workers (part 1)

d5d9b63

Update ktdoc

a7e9575

MediaUploadWorker cleanup WIP

61cabf0

MediaUploadWorker cleanup WIP

f6b2e2d

MediaUploadWorker cleanup WIP

91aed7b

MediaUploadWorker cleanup WIP

96f49a2

MediaUploadWorker cleanup WIP

a892eab

Minor refactors

72af405

Remove unused import

c06a3c8

Media worker - process entire queue

cd7f219

Minor readability improvements

d1e2185

Clean up unused

ee0c43f

Add TODO

9e8549a

Add media mutation class

e5ece5f

Revert addition of MediaMutation

3e06b48

gino-m changed the title ~~[Data sync] Try to process entire queue each time worker is run~~ [Data sync] Major rewrite of data sync workers Nov 26, 2024

gino-m added 3 commits November 26, 2024 13:25

Refactor all the things

d6cdbd5

Make method private

3c1230b

Remove unused funs

1aa44b9

gino-m added 2 commits November 27, 2024 15:19

Log network issues as debug

a1c4f7d

Tweak workaround

356e022

gino-m marked this pull request as ready for review November 27, 2024 20:25

auto-assign bot requested review from scolsen and sufyanAbbasi November 27, 2024 20:25

auto-assign bot assigned anandwana001 and shobhitagarwal1612 Nov 27, 2024

gino-m requested review from shobhitagarwal1612 and anandwana001 and removed request for scolsen and sufyanAbbasi November 27, 2024 20:25

gino-m added 3 commits November 27, 2024 15:28

Small refactor and TODO

ee7ca26

Rename function

3d214cd

Tweak TODO

1776442

shobhitagarwal1612 requested changes Nov 28, 2024

View reviewed changes

shobhitagarwal1612 reviewed Nov 28, 2024

View reviewed changes

ground/src/main/java/com/google/android/ground/model/submission/UploadQueueEntry.kt Outdated Show resolved Hide resolved

gino-m added 3 commits December 2, 2024 10:25

Merge branch 'master' of https://github.com/google/ground-android int…

294c334

…o gino-m/2840/improve-upload-queue-1

Tweak ktdoc

b0e0fdf

Alphabetize constructor args

5e669ec

gino-m commented Dec 2, 2024

View reviewed changes

gino-m and others added 2 commits December 2, 2024 10:45

Merge branch 'master' into gino-m/2840/improve-upload-queue-1

4308ffa

Merge branch 'gino-m/2840/improve-upload-queue-1' of https://github.c…

16944dc

…om/google/ground-android into gino-m/2840/improve-upload-queue-1

shobhitagarwal1612 reviewed Dec 2, 2024

View reviewed changes

ground/src/main/java/com/google/android/ground/repository/MutationRepository.kt Outdated Show resolved Hide resolved

shobhitagarwal1612 approved these changes Dec 2, 2024

View reviewed changes

gino-m added 2 commits December 2, 2024 14:27

Remove unused constant

6250fe0

Add explicit type

a3cd4bf

gino-m merged commit 9ab91bc into master Dec 2, 2024
4 checks passed

gino-m deleted the gino-m/2840/improve-upload-queue-1 branch December 2, 2024 20:20

gino-m mentioned this pull request Dec 2, 2024

[Data sync] Make data sync less error prone and easier to maintain #2840

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data sync] Rewrite data and photo sync workers to be more robust and readable #2867

[Data sync] Rewrite data and photo sync workers to be more robust and readable #2867

gino-m commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

gino-m commented Nov 27, 2024

shobhitagarwal1612 left a comment

shobhitagarwal1612 Nov 28, 2024

gino-m Dec 2, 2024

shobhitagarwal1612 Nov 28, 2024

gino-m Dec 2, 2024

gino-m Dec 2, 2024

gino-m left a comment

gino-m Dec 2, 2024

gino-m Dec 2, 2024

[Data sync] Rewrite data and photo sync workers to be more robust and readable #2867

[Data sync] Rewrite data and photo sync workers to be more robust and readable #2867

Conversation

gino-m commented Nov 22, 2024 • edited Loading

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

gino-m commented Nov 27, 2024

shobhitagarwal1612 left a comment

Choose a reason for hiding this comment

shobhitagarwal1612 Nov 28, 2024

Choose a reason for hiding this comment

gino-m Dec 2, 2024

Choose a reason for hiding this comment

shobhitagarwal1612 Nov 28, 2024

Choose a reason for hiding this comment

gino-m Dec 2, 2024

Choose a reason for hiding this comment

gino-m Dec 2, 2024

Choose a reason for hiding this comment

gino-m left a comment

Choose a reason for hiding this comment

gino-m Dec 2, 2024

Choose a reason for hiding this comment

gino-m Dec 2, 2024

Choose a reason for hiding this comment

gino-m commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading