Skip to content
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

Start writing new commits/tasks to Firestore #3472

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

keyonghan
Copy link
Contributor

This PR

  1. starts writing new commit/tasks to the new Firestore database
  2. is a no-op to existing workflow. Will catch any exception without breaking existing logic.
  3. puts generic logics (like documents to writes, comits/tasks to documents, etc) in the firestore service
  4. adds extensive tests for each logic added.

Part of flutter/flutter#142951.

/// This is an atomic operation: either all writes succeed or all writes fail.
Future<CommitResponse> writeViaTransaction(List<Write> writes) async {
final ProjectsDatabasesDocumentsResource databasesDocumentsResource = await documentResource();
final BeginTransactionRequest beginTransactionRequest =
Copy link
Contributor

Choose a reason for hiding this comment

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

Some databases have multiple commit modes. Is there anything in Firestore where you don't have to manage the commits manually like with with transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a transaction operation, a commit is explicitly needed. There are other operations like batchWrites, which doesn't need a commit.

BeginTransactionRequest(options: TransactionOptions(readWrite: ReadWrite()));
final BeginTransactionResponse beginTransactionResponse =
await databasesDocumentsResource.beginTransaction(beginTransactionRequest, kDatabase);
final CommitRequest commitRequest =
Copy link
Contributor

Choose a reason for hiding this comment

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

If commit fails do we need a rollback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Transaction acts as an atomic operation. If it fails, everything will auto roll back.

List<Document> targetsToTaskDocuments(Commit commit, List<Target> targets) {
final Iterable<Document> iterableDocuments = targets.map(
(Target target) => Document(
name: '$kDatabase/documents/tasks/${commit.sha}_${target.value.name}_1',
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the significance of the '_1'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using _1 to track the old attempt logic. We want to keep all the records of retries, instead of overriding using a single record.

(Target target) => Document(
name: '$kDatabase/documents/tasks/${commit.sha}_${target.value.name}_1',
fields: <String, Value>{
'builderNumber': Value(integerValue: null),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this can be nullable? Is this the main identifier?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this might be updated at a later date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. We will update the entry later when status changes.

Copy link
Contributor

@ricardoamador ricardoamador left a comment

Choose a reason for hiding this comment

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

LGTM!

@keyonghan keyonghan added the autosubmit Merge PR when tree becomes green via auto submit App. label Feb 7, 2024
@auto-submit auto-submit bot merged commit 62d724e into flutter:main Feb 8, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App.
Projects
None yet
3 participants