Make schemas back and forth compatible #1552

ashwinraghav · 2020-05-11T23:15:14Z

#1548 evidences issues that we have with our Schema Manager when customers go back and forth between versions (here on referred to as Version Fluidity) Thanks @rafikhan .

These are our options:

We can either take Firestore's stand of being lossless when customers migrate back and forth between versions.
We can take a systematic cleanup approach that deletes all state when clients are migrated to older versions.

We are leaning towards option 2

Given Firestore' s approach to preserve state, the advice from the API council was to take a similar approach.
Deleting old data might make in intractable to use Firelog for some use cases going foward.

Design considerations
We have modified migrations (history), which is typically a red flag. However since we have modeled db creation as running all migrations(replaying history), there's not a cleaner way forward.

Modifications to existing migrations involves one of two things

Creating tables only if they don't exist.
Altering columns if they exist. Since there's no sql construct, we catch exceptions around the operation.

Version compatibility

The migrations and tests are no longer representative of this compatibility since we essentially changing history.

Version Fluidity matrix

	1	2	3	4
1	😐	✅	✅	❌
2	✅	😐	✅	❌
3	✅	✅	😐	❌
4	❌	❌	❌	😐

google-oss-bot · 2020-05-11T23:18:31Z

Coverage Report

Affected SDKs

transport-runtime

SDK overall coverage changed from 58.53% (2f3447f) to 58.58% (fe8f6ea1) by +0.04%.

Filename Base (2f3447f) Head (fe8f6ea1) Diff

SQLiteEventStore.java 51.46% 51.09% -0.36%

SchemaManager.java 98.08% 98.21% +0.14%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (fe8f6ea1) is created by Prow via merging commits: 2f3447f 7c00efb.

google-oss-bot · 2020-05-11T23:20:46Z

Binary Size Report

Affected SDKs

transport-api

Type Base (2f3447f) Head (fe8f6ea1) Diff

apk (aggressive) 11.0 kB 11.0 kB +2 B (+0.0%)

apk (debug) 23.0 kB 23.0 kB +16 B (+0.1%)

`transport-runtime`

Type	Base (`2f3447f`)	Head (fe8f6ea1)	Diff
aar	127 kB	127 kB	+59 B (+0.0%)
apk (aggressive)	35.6 kB	35.6 kB	+17 B (+0.0%)
apk (debug)	80.5 kB	80.6 kB	+97 B (+0.1%)
apk (release)	62.8 kB	62.9 kB	+99 B (+0.2%)

Test Logs

Notes

Head commit (fe8f6ea1) is created by Prow via merging commits: 2f3447f 7c00efb.

google-oss-bot · 2020-05-18T22:54:33Z

@ashwinraghav: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
build-plugins-check	`5edc500`	link	`/test build-plugins-check`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

...main/java/com/google/android/datatransport/runtime/scheduling/persistence/SchemaManager.java

vkryachko

Marking "Request changes" to prevent accidental merge, let's discuss first, specifically the option to "drop if exists" when migrating to v4(it seems that it will resolve all issues)

...main/java/com/google/android/datatransport/runtime/scheduling/persistence/SchemaManager.java

ashwinraghav · 2020-05-26T22:09:06Z

#1548 evidences issues that we have with our Schema Manager when customers go back and forth between versions (here on referred to as Version Fluidity) Thanks @rafikhan .

These are our options:

We can either take Firestore's stand of being lossless when customers migrate back and forth between versions.

We can take a systematic cleanup approach that deletes all state when clients are migrated to older versions.

We are leaning towards option 2

Given Firestore' s approach to preserve state, the advice from the API council was to take a similar approach.

Deleting old data might make in intractable to use Firelog for some use cases going foward.

Design considerations
We have modified migrations (history), which is typically a red flag. However since we have modeled db creation as running all migrations(replaying history), there's not a cleaner way forward.

Modifications to existing migrations involves one of two things

Creating tables only if they don't exist.

Altering columns if they exist. Since there's no sql construct, we catch exceptions around the operation.

Version compatibility

The migrations and tests are no longer representative of this compatibility since we essentially changing history.

Version Fluidity matrix

1 2 3 4
1 😐 ✅ ✅ ❌
2 ✅ 😐 ✅ ❌
3 ✅ ✅ 😐 ❌
4 ❌ ❌ ❌ 😐

We've decided to minimize the scope of changes for now and roll forward with just dropping the table before creation

mkolodiiproexe · 2020-06-09T06:09:23Z

@vkryachko could you please let me know, when these fixes will be released in production?
I'm currently having the issue happening with Analytics 17.4.3.

Fatal Exception: android.database.sqlite.SQLiteException: table event_payloads already exists (code 1 SQLITE_ERROR): , while compiling: CREATE TABLE event_payloads (sequence_num INTEGER NOT NULL, event_id INTEGER NOT NULL, bytes BLOB NOT NULL,FOREIGN KEY (event_id) REFERENCES events(_id) ON DELETE CASCADE,PRIMARY KEY (sequence_num, event_id))
       at android.database.sqlite.SQLiteConnection.nativePrepareStatement(SQLiteConnection.java)
       at android.database.sqlite.SQLiteConnection.acquirePreparedStatement(SQLiteConnection.java:986)
       at android.database.sqlite.SQLiteConnection.prepare(SQLiteConnection.java:593)
       at android.database.sqlite.SQLiteSession.prepare(SQLiteSession.java:590)
       at android.database.sqlite.SQLiteProgram.<init>(SQLiteProgram.java:61)
       at android.database.sqlite.SQLiteStatement.<init>(SQLiteStatement.java:33)
       at android.database.sqlite.SQLiteDatabase.executeSql(SQLiteDatabase.java:1805)
       at android.database.sqlite.SQLiteDatabase.execSQL(SQLiteDatabase.java:1733)
       at com.google.android.datatransport.runtime.scheduling.persistence.SchemaManager.lambda$static$3(SchemaManager.java:109)
       at com.google.android.datatransport.runtime.scheduling.persistence.SchemaManager$$Lambda$4.upgrade(SchemaManager.java)
       at com.google.android.datatransport.runtime.scheduling.persistence.SchemaManager.upgrade(SchemaManager.java:182)
       at com.google.android.datatransport.runtime.scheduling.persistence.SchemaManager.onUpgrade(SchemaManager.java:152)
       at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:417)
       at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:317)
       at com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore$$Lambda$1.produce(SQLiteEventStore.java:2)
       at com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore.retryIfDbLocked(SQLiteEventStore.java:517)
       at com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore.getDb(SQLiteEventStore.java:82)
       at com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore.runCriticalSection(SQLiteEventStore.java:549)
       at com.google.android.datatransport.runtime.scheduling.jobscheduling.WorkInitializer.lambda$ensureContextsScheduled$1(WorkInitializer.java:54)
       at com.google.android.datatransport.runtime.scheduling.jobscheduling.WorkInitializer$$Lambda$1.run(WorkInitializer.java:2)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
       at java.lang.Thread.run(Thread.java:919)

vkryachko · 2020-06-09T15:16:23Z

@mkolodiiproexe The fix is out as part of com.google.firebase:firebase-datatransport:17.0.6 and the latest crashlytics depend on it. Can you provide a list of your app deps to help figure out why you're seeing this?

mirokolodii · 2020-06-09T18:35:31Z

@vkryachko I was not able to find that particular dependency. Can you access this link? https://scans.gradle.com/s/rblcgiyzryidc?
It includes all the dependencies from my app
https://scans.gradle.com/s/rblcgiyzryidc/dependencies?toggled=W1swXSxbMCw3XV0

vkryachko · 2020-06-09T19:50:47Z

sorry, the actual SDK that contained this bug was transport-runtime
I can see that you have the updated version of it 2.2.3 which has this fix, so it should not be happening.
@ashwinraghav any thoughts?

ashwinraghav · 2020-06-09T21:33:46Z

@mkolodiiproexe these crashes are unrelated to analytics. If you meant crashlytics, they are already available in firebase-crashlytics:17.0.1

mirokolodii · 2020-06-10T04:29:28Z

@ashwinraghav ok, it may be crashlytics. It just started to happen when I added analytics lib. But anyway, how can it be fixed?

googlebot added the cla: yes Override cla label May 11, 2020

google-oss-bot added the size/L label May 11, 2020

ashwinraghav changed the base branch from master to schemaManagerTest May 11, 2020 23:18

google-oss-bot added size/M and removed size/L labels May 11, 2020

ashwinraghav force-pushed the schemaManagerFix branch from 394a7c3 to 76920cf Compare May 11, 2020 23:23

google-oss-bot added size/L and removed size/M labels May 11, 2020

ashwinraghav force-pushed the schemaManagerFix branch from 76920cf to 3f74e10 Compare May 15, 2020 17:21

google-oss-bot added size/XXL and removed size/L labels May 15, 2020

ashwinraghav force-pushed the schemaManagerFix branch from 3f74e10 to 521bc71 Compare May 15, 2020 17:22

ashwinraghav changed the base branch from schemaManagerTest to master May 15, 2020 17:23

google-oss-bot added size/L and removed size/XXL labels May 15, 2020

ashwinraghav force-pushed the schemaManagerFix branch from 521bc71 to 5edc500 Compare May 18, 2020 22:53

google-oss-bot added size/XXL and removed size/L labels May 18, 2020

ashwinraghav requested a review from vkryachko May 18, 2020 22:53

ashwinraghav added 3 commits May 18, 2020 15:53

Added tests to reason about backward compatibility

fb99d33

Added state simulations

c0e647c

Making all migrations backward compatible

e97855c

ashwinraghav force-pushed the schemaManagerFix branch from 5edc500 to e97855c Compare May 18, 2020 22:54

google-oss-bot added size/L and removed size/XXL labels May 18, 2020

GoogleJavaFormat

65ab1bd

ashwinraghav linked an issue May 18, 2020 that may be closed by this pull request

SQLiteException in firebase-crashlytics #1531

Closed

This was linked to issues May 18, 2020

SQLiteException on event_payloads table #1567

Closed

Runtime SQLite Table crash after upgrading Core and Messaging Versions #1568

Closed

LeeOhHyung reviewed May 25, 2020

View reviewed changes

...main/java/com/google/android/datatransport/runtime/scheduling/persistence/SchemaManager.java Outdated Show resolved Hide resolved

davidmotson approved these changes May 26, 2020

View reviewed changes

rlazo approved these changes May 26, 2020

View reviewed changes

vkryachko requested changes May 26, 2020

View reviewed changes

ashwinraghav added 3 commits May 26, 2020 14:47

minimizing change scope

0422a0d

Removing alters

a82b6f8

dropping if exists

7c00efb

ashwinraghav requested a review from vkryachko May 26, 2020 22:25

vkryachko approved these changes May 26, 2020

View reviewed changes

vkryachko merged commit 376c25c into master May 26, 2020

ashwinraghav mentioned this pull request Jun 11, 2020

Fatal Exception: android.database.sqlite.SQLiteException: table event_payloads already exists #1640

Closed

firebase locked and limited conversation to collaborators Jun 26, 2020

kaibolay deleted the schemaManagerFix branch September 14, 2022 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make schemas back and forth compatible #1552

Make schemas back and forth compatible #1552

ashwinraghav commented May 11, 2020 •

edited

Loading

google-oss-bot commented May 11, 2020 •

edited

Loading

`transport-runtime`

google-oss-bot commented May 11, 2020 •

edited

Loading

`transport-api`

`transport-runtime`

google-oss-bot commented May 18, 2020 •

edited

Loading

vkryachko left a comment

ashwinraghav commented May 26, 2020

Version compatibility

mkolodiiproexe commented Jun 9, 2020

vkryachko commented Jun 9, 2020

mirokolodii commented Jun 9, 2020 •

edited

Loading

vkryachko commented Jun 9, 2020

ashwinraghav commented Jun 9, 2020

mirokolodii commented Jun 10, 2020

Make schemas back and forth compatible #1552

Make schemas back and forth compatible #1552

Conversation

ashwinraghav commented May 11, 2020 • edited Loading

Version compatibility

google-oss-bot commented May 11, 2020 • edited Loading

Coverage Report

Affected SDKs

transport-runtime

Test Logs

Notes

google-oss-bot commented May 11, 2020 • edited Loading

Binary Size Report

Affected SDKs

transport-api

transport-runtime

Test Logs

Notes

google-oss-bot commented May 18, 2020 • edited Loading

vkryachko left a comment

Choose a reason for hiding this comment

ashwinraghav commented May 26, 2020

Version compatibility

mkolodiiproexe commented Jun 9, 2020

vkryachko commented Jun 9, 2020

mirokolodii commented Jun 9, 2020 • edited Loading

vkryachko commented Jun 9, 2020

ashwinraghav commented Jun 9, 2020

mirokolodii commented Jun 10, 2020

ashwinraghav commented May 11, 2020 •

edited

Loading

google-oss-bot commented May 11, 2020 •

edited

Loading

`transport-runtime`

google-oss-bot commented May 11, 2020 •

edited

Loading

`transport-api`

`transport-runtime`

google-oss-bot commented May 18, 2020 •

edited

Loading

mirokolodii commented Jun 9, 2020 •

edited

Loading