Skip to content

Add transaction retries.#683

Merged
godofredoc merged 2 commits intoflutter:masterfrom
godofredoc:fix_backend_errors
Mar 11, 2020
Merged

Add transaction retries.#683
godofredoc merged 2 commits intoflutter:masterfrom
godofredoc:fix_backend_errors

Conversation

@godofredoc
Copy link
Contributor

@godofredoc godofredoc commented Mar 11, 2020

Most of the 500 errors in the backend are related to failing
transactions. This change adds retries with quadratic backoffs to remove
the flakiness.

Bugs:

flutter/flutter#42524
flutter/flutter#43112
flutter/flutter#49673
flutter/flutter#49672

CaseyHillers
CaseyHillers approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@CaseyHillers CaseyHillers left a comment

Choose a reason for hiding this comment

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

LGTM with nit! This is a great utility function to have available in the backend.

app_dart/lib/src/foundation/utils.dart Outdated
Copy link
Contributor

@CaseyHillers CaseyHillers Mar 11, 2020

Choose a reason for hiding this comment

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

I think we should add these as optional parameters to the function with these as the default values. It could be useful to modify these if there are some operations we want to modify this on.

Copy link
Contributor Author

@godofredoc godofredoc Mar 11, 2020

Choose a reason for hiding this comment

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

Done, added them as optional parameters.

keyonghan
keyonghan approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

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

LGTM!

@CaseyHillers CaseyHillers mentioned this pull request Mar 11, 2020
Merged
digiter
digiter approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@digiter digiter left a comment

Choose a reason for hiding this comment

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

LGTM with nits: I would try to reduce the number of nesting scopes, to improve readability.

@keyonghan keyonghan mentioned this pull request Mar 11, 2020
Closed
@godofredoc
Add transaction retries.
6a37f68 
Most of the 500 errors in the backend are related to failing
transactions. This change adds retries with quadratic backoffs to remove
the flakiness.

Bugs:

flutter/flutter#42524
flutter/flutter#43112
flutter/flutter#49673
flutter/flutter#49672
@godofredoc
Add tests and also retry on grpc errors.
01164b7
@godofredoc
Copy link
Contributor Author

godofredoc commented Mar 11, 2020

@digiter would you mind expanding on reducing the nesting scope? is it within the new function or nesting in general where the function is being used?

@CaseyHillers
Copy link
Contributor

CaseyHillers commented Mar 11, 2020

@godofredoc one way I imagine reducing the nesting scope it to have datastore.dart implement it's own withTransaction function and we switch the Cocoon usage to use that function.

For example this PR has:

await runTransactionWithRetries(() async {
          await datastore.db
              .withTransaction<void>((Transaction transaction) async {
            final TimeSeries series =
                await _getOrCreateTimeSeries(transaction, task, scoreKey);
            final num value = resultData[scoreKey] as num;

            final TimeSeriesValue seriesValue = TimeSeriesValue(
              key: series.key.append(TimeSeriesValue),
              createTimestamp: DateTime.now().millisecondsSinceEpoch,
              revision: commit.sha,
              taskKey: task.key,
              value: value.toDouble(),
            );

            transaction.queueMutations(inserts: <TimeSeriesValue>[seriesValue]);
            await transaction.commit();
          });
        });

instead we could do:

final TimeSeries series = await _getOrCreateTimeSeries(transaction, task, scoreKey);
final num value = resultData[scoreKey] as num;
final TimeSeriesValue seriesValue = TimeSeriesValue(
    key: series.key.append(TimeSeriesValue),
    createTimestamp: DateTime.now().millisecondsSinceEpoch,
    revision: commit.sha,
    taskKey: task.key,
    value: value.toDouble(),
);
await datastore.withTransaction<void>(insert: <TimeSeriesValue>[seriesValue]); // this being the end goal of removing the extra nesting

digiter
digiter reviewed Mar 11, 2020
View reviewed changes
app_dart/lib/src/request_handlers/refresh_chromebot_status.dart
}
});
});
}
Copy link
Contributor

@digiter digiter Mar 11, 2020

Choose a reason for hiding this comment

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

@godofredoc I'm talking about reducing nesting in general, for example, this change has 6 nested scopes, which made it a little difficult to read. Also see go/tott/455.

Copy link
Contributor Author

@godofredoc godofredoc Mar 11, 2020

Choose a reason for hiding this comment

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

I see what you mean, thanks for the explanation. Those several layers of nested scopes are everywhere where the backend is using transactions.

Added a new bug for the cleanup as it will require to update most of the handlers:

flutter/flutter#52436

@godofredoc
Copy link
Contributor Author

godofredoc commented Mar 11, 2020

@CaseyHillers thanks for the explanation added flutter/flutter#52436 for a follow up cl on cleaning up the transactional code in the backend.

@godofredoc godofredoc merged commit 1c1da68 into flutter:master Mar 11, 2020
@godofredoc godofredoc deleted the fix_backend_errors branch May 9, 2020 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@digiter digiter digiter approved these changes

@CaseyHillers CaseyHillers CaseyHillers approved these changes

@keyonghan keyonghan keyonghan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@godofredoc @CaseyHillers @digiter @keyonghan