add uploader #327

VinayGuthal · 2019-03-29T20:04:36Z

Add uploader which the scheduler will use to upload to our sqlite database.

...t-runtime/src/main/java/com/google/android/datatransport/runtime/TransportRuntimeModule.java

...rc/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java

ashwinraghav · 2019-03-30T00:02:43Z

...rc/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java

+  void logAndUpdateState(String backendName, int attemptNumber) {
+    TransportBackend backend = backendRegistry.get(backendName);
+    List<EventInternal> eventInternals = new ArrayList<>();
+    Iterable<PersistedEvent> persistedEvents =


If I understand correctly, the explicit synchronization is only required because we want to support multi process thread safety ?

Questions

The API for the datastore seems to require that call sites responsibly call eventStore methods in critical sections. I see that this cannot be done within the implementations of the eventStore since we invoke others apis from within critical sections, that are outside of the store. Would a single operation on datastore have to be run in a critical section too?

If the critical sections are scattered across the project, I would vote to bring them all into a single file and encapsulate them as higher level functions

require that call sites responsibly call eventStore methods in critical sections

It's not the case, you can call them by themselves they will just be part of their own isolated transaction, the reason you'd want to call them within a critical section is to guarantee atomicity.

iiu your comment correctly runCriticalSection is exactly the higher-level function you're describing, so I am not sure what the suggestion is here, can you be more specific, e.g. provide an example?

Ok. So it seems like each operation on datastore itself need not be in a critical section. THis is requires only when multiple operations are to be atomic.

What I mean is -> All invocations of runCriticalSection could be centralized. We will have a single place to reason about all higher level operations that we need to be atomic. Is this file that place or are there other files that invoke runCtiticalSection ?

So we have to places that need to synchronize:

Event producing "side", namely transport.send(event)

Event uploading "side"

I would prefer not to place everything "synchronization related" to a single file/class. The reason is: synchronization is a cross-cutting concern, namely not related to business logic but rather an implementation detail that crosses the boundaries of different domains(storage, scheduling, uploading). For this reason there is no good central place to put synchronization and doing so would result in a "god-object" that does everything. I don't think it is justified in general and in our case in particular, as we have just 2 places that need synchronization. wdyt?

There are other files which invoke runCriticalSection as well. Like the DefaultScheduler.java does the schedule and the decorate in one critical section

@VinayGuthal this is what I was referring to in 1. Namely, this is what's called as a result of transport.send(event).

Ok. Defer to your judgement

...rc/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java

ashwinraghav · 2019-04-04T18:07:32Z

...rc/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java

+  void logAndUpdateState(String backendName, int attemptNumber) {
+    TransportBackend backend = backendRegistry.get(backendName);
+    List<EventInternal> eventInternals = new ArrayList<>();
+    Iterable<PersistedEvent> persistedEvents =


Ok. Defer to your judgement

VinayGuthal · 2019-04-04T20:41:25Z

/test connected-check-changed

transport/transport-runtime/src/main/AndroidManifest.xml

vkryachko · 2019-04-04T22:10:18Z

...t-runtime/src/main/java/com/google/android/datatransport/runtime/TransportRuntimeModule.java

@@ -67,6 +68,17 @@ static WorkScheduler workScheduler(
    }
  }

+  @Provides
+  static Uploader uploader(


You can remove this as the uploader is annotated with @Inject

Also please mark the executor above as @Singleton

You can remove this as the uploader is annotated with @Inject
Also please mark the executor above as @Singleton

These 2 are still not done, please change

...rc/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java

...java/com/google/android/datatransport/runtime/scheduling/jobscheduling/JobInfoScheduler.java

vkryachko · 2019-04-05T20:48:10Z

transport/transport-runtime/src/main/AndroidManifest.xml

+        <service
+            android:name=".scheduling.jobscheduling.JobInfoSchedulerService"
+            android:label="JobScheduler Service"
+            android:permission="android.permission.BIND_JOB_SERVICE" >


please add android:exported="false", also it seems we don't need the label(none of our services/content-providers are labeled)

vkryachko · 2019-04-05T20:48:57Z

...t-runtime/src/main/java/com/google/android/datatransport/runtime/TransportRuntimeModule.java

@@ -67,6 +68,17 @@ static WorkScheduler workScheduler(
    }
  }

+  @Provides
+  static Uploader uploader(


You can remove this as the uploader is annotated with @Inject
Also please mark the executor above as @Singleton

These 2 are still not done, please change

add uploader

a6f7860

VinayGuthal requested review from vkryachko and ashwinraghav March 29, 2019 20:04

googlebot added the cla: yes Override cla label Mar 29, 2019

google-oss-bot added the size/L label Mar 29, 2019

VinayGuthal added 2 commits March 29, 2019 16:27

gJF

715119f

test callback

3ed0357

ashwinraghav suggested changes Mar 30, 2019

View reviewed changes

vkryachko reviewed Mar 30, 2019

View reviewed changes

...rc/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java Outdated Show resolved Hide resolved

VinayGuthal added 3 commits April 1, 2019 15:04

put schedule in a critical section

b98df16

fix

122f8f6

remove null check

fe62fe5

ashwinraghav approved these changes Apr 4, 2019

View reviewed changes

VinayGuthal added 4 commits April 4, 2019 14:10

Merge branch 'master' into add_uploader

f24aebd

Merge branch 'master' into add_uploader

c2a692c

Merge branch 'master' into add_uploader

0f66ad6

merge commits

9fb39a6

vkryachko requested changes Apr 4, 2019

View reviewed changes

VinayGuthal added 5 commits April 5, 2019 14:00

update uploader tests

9b39e97

use a different clock

c73b206

address comments

4960192

update uploader

41833ed

fix idea

d41f320

VinayGuthal force-pushed the add_uploader branch from 368de01 to d41f320 Compare April 5, 2019 20:54

vkryachko requested changes Apr 5, 2019

View reviewed changes

VinayGuthal added 2 commits April 5, 2019 17:36

address comments

f3ac278

address comments

7c574cc

vkryachko approved these changes Apr 8, 2019

View reviewed changes

VinayGuthal merged commit 410feec into master Apr 8, 2019

VinayGuthal deleted the add_uploader branch April 8, 2019 14:29

firebase locked and limited conversation to collaborators Oct 11, 2019

add uploader #327

add uploader #327

Uh oh!

Conversation

VinayGuthal commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkryachko Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VinayGuthal commented Apr 4, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VinayGuthal commented Mar 29, 2019 •

edited

Loading

vkryachko Apr 1, 2019 •

edited

Loading