Skip to content

Port "Stop persisting last acknowledged batch ID" from iOS #188

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 12, 2019
Merged

Port "Stop persisting last acknowledged batch ID" from iOS #188

merged 4 commits into from
Jan 12, 2019

Conversation

@var-const
Copy link
Contributor

@var-const var-const commented Jan 11, 2019

A straightforward port of firebase/firebase-ios-sdk#2243.

@googlebot googlebot added the cla: yes Override cla label Jan 11, 2019
@var-const var-const requested a review from mikelehen January 11, 2019 19:25
@var-const
Copy link
Contributor Author

var-const commented Jan 11, 2019

/retest

mikelehen
mikelehen approved these changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except perhaps add a comment to SQLiteSchema.java that last_acknowledged_batch_id is no longer used?

@mikelehen mikelehen assigned var-const and unassigned mikelehen Jan 11, 2019
@var-const
Copy link
Contributor Author

var-const commented Jan 11, 2019

/test smoke-tests-debug

@var-const
Copy link
Contributor Author

var-const commented Jan 11, 2019

add a comment to SQLiteSchema.java that last_acknowledged_batch_id is no longer used?

Added above the migration to remove acknowledged mutations, let me know if I should move it to a more prominent place.

@var-const
Copy link
Contributor Author

var-const commented Jan 11, 2019

/retest

1 similar comment
@wilhuff
Copy link
Contributor

wilhuff commented Jan 11, 2019

/retest

@wilhuff
Copy link
Contributor

wilhuff commented Jan 11, 2019

/test smoke-tests-debug

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jan 11, 2019

@var-const: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
smoke-tests-debug 6c8a600 link /test smoke-tests-debug
smoke-tests-release 6c8a600 link /test smoke-tests-release

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.

@wilhuff
Copy link
Contributor

wilhuff commented Jan 12, 2019

Database smoke tests are known flaky and we haven't yet constrained the smoke-tests-* tasks to run only affected changes so proceeding.

@wilhuff wilhuff merged commit 5187d09 into master Jan 12, 2019
@wilhuff wilhuff deleted the varconst/eliminate-highest-ackd-batch-id branch January 12, 2019 00:16
@firebase firebase locked and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mikelehen mikelehen mikelehen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@var-const var-const

Labels

cla: yes Override cla size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@var-const @wilhuff @google-oss-bot @mikelehen @googlebot