Add Retry Logic to Airbyte API calls. #19693

davinchia · 2022-11-22T03:24:12Z

What

Today we often see HTTP/1.1 header parser received no bytes' during syncs, especially in the Data Plane.

This PR attempts to fix this by adding naive retries.

~~If this works, we will explore using a more fully featured HTTP Client. I will not merge this in.~~

It turns out this is the best way for what we are trying to do. More details in How section.

How

Add a basic retry wrapper with the unique ability to retry for a much longer period on the last retry. This is particularly useful for us as most of our jobs are long running workflows, and the benefit of not having to restart the entire job outweighs the added wait time.

Alternative solutions I explored:

Switching the underlying HTTP client to a more fully featured HTTP client. E.g. Apache or OkHttp. Issues with this:
- These clients do not support the ability to configure the retry policy we want.
- These clients do not support the ability to inject application aware logging.
- Most importantly, because this changes the interface, the resulting change set is big and affects many unrelated classes. I do think we eventually want to switch the underlying libraries out. However I don't think we should do this as part of OC work.
Exploring pairing retry libraries such as https://resilience4j.readme.io/docs with the native http clients. The main issue here is the lack of ability to configure the last retry period.

Since the hand-rolled wrapper is simple + gets the job done, my thoughts are to run with this for the time being and revisit this if additional requirements around the clients come up.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

airbyte-workers/src/main/java/io/airbyte/workers/temporal/TemporalAttemptExecution.java

… all the spots we call the api.

davinchia · 2022-11-24T06:23:05Z

I have confirmed this fix works. See https://github.com/airbytehq/oncall/issues/1079#issuecomment-1324451000 for more info.

Going to leave this open when I explore a cleaner fix.

davinchia · 2022-11-24T18:22:27Z

airbyte-workers/src/main/java/io/airbyte/workers/temporal/sync/NormalizationActivityImpl.java

    final JobIdRequestBody id = new JobIdRequestBody();
    id.setId(Long.valueOf(jobRunConfig.getJobId()));
-    final var jobScope = airbyteApiClient.getJobsApi().getJobInfo(id).getJob().getConfigId();
+    final var jobScope = AirbyteApiClient.retryWithJitter(
+        () -> airbyteApiClient.getJobsApi().getJobInfo(id).getJob().getConfigId(),


@pmossman I feel like we can switch this to getJobInfoLight?

If so, will do in a follow up PR.

davinchia · 2022-11-24T18:22:31Z

...te-workers/src/main/java/io/airbyte/workers/temporal/sync/DbtTransformationActivityImpl.java

-    final var jobScope = airbyteApiClient.getJobsApi().getJobInfo(id).getJob().getConfigId();
+
+    final var jobScope = AirbyteApiClient.retryWithJitter(
+        () -> airbyteApiClient.getJobsApi().getJobInfo(id).getJob().getConfigId(),


davinchia · 2022-11-24T18:23:27Z

PTAL @pmossman @xiaohansong ended up going with the hand-rolled solution for the reasons in the PR description.

xiaohansong

Some minor comments, otherwise LGTM

airbyte-api/src/main/java/io/airbyte/api/client/AirbyteApiClient.java

xiaohansong · 2022-11-24T21:22:07Z

airbyte-workers/src/main/java/io/airbyte/workers/temporal/TemporalAttemptExecution.java

-      saveWorkflowIdForCancellation(airbyteApiClient);
+      AirbyteApiClient.retryWithJitter(() -> {
+        saveWorkflowIdForCancellation(airbyteApiClient);
+        return null;


Can we remove this return null?

I looked into this and I think the alternatives are messier.

We can turn the interface into a Runnable, which doesn't expect an return value. However Runnables do not support checked exceptions, which means we would have to catch and recast the exception thrown by saveWorkflowIdForCancellation into a RTE.

Given that, I think it's simpler to keep this as is!

If you have a suggestion, I'm happy to edit this!

airbyte-api/src/main/java/io/airbyte/api/client/AirbyteApiClient.java

Add application logic to retry api calls.

982f7da

octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker labels Nov 22, 2022

davinchia requested review from xiaohansong and pmossman November 22, 2022 03:24

davinchia commented Nov 22, 2022

View reviewed changes

airbyte-workers/src/main/java/io/airbyte/workers/temporal/TemporalAttemptExecution.java Outdated Show resolved Hide resolved

Sleep for ten mins on the last attempt.

623464a

davinchia temporarily deployed to more-secrets November 22, 2022 21:20 Inactive

davinchia added 2 commits November 22, 2022 15:28

Pull jitter out into a separate method. Inject retry with jitter into…

0ff8813

… all the spots we call the api.

Merge branch 'master' into davinchia/retry-client

9e92aa6

davinchia temporarily deployed to more-secrets November 22, 2022 23:31 Inactive

davinchia changed the title ~~Test application logic retry calls.~~ DNM: Test application logic retry calls. Nov 24, 2022

Retry to make more usable.

ae94145

octavia-squidington-iv added the area/api Related to the api label Nov 24, 2022

davinchia temporarily deployed to more-secrets November 24, 2022 17:18 Inactive

davinchia changed the title ~~DNM: Test application logic retry calls.~~ Add Retry Logic to Airbyte API calls. Nov 24, 2022

Pass PMD and check.

0b7741d

davinchia temporarily deployed to more-secrets November 24, 2022 17:51 Inactive

Better comments.

c744b02

davinchia temporarily deployed to more-secrets November 24, 2022 18:11 Inactive

Better comments.

44e8863

davinchia temporarily deployed to more-secrets November 24, 2022 18:20 Inactive

davinchia marked this pull request as ready for review November 24, 2022 18:20

davinchia commented Nov 24, 2022

View reviewed changes

xiaohansong approved these changes Nov 24, 2022

View reviewed changes

davinchia temporarily deployed to more-secrets November 25, 2022 00:04 Inactive

Fix comment. Fix Math.max.

ca0ddbb

davinchia temporarily deployed to more-secrets November 25, 2022 05:41 Inactive

davinchia temporarily deployed to more-secrets November 26, 2022 17:29 Inactive

davinchia merged commit a695b41 into master Nov 28, 2022

davinchia deleted the davinchia/retry-client branch November 28, 2022 16:44

github-actions bot mentioned this pull request Nov 28, 2022

Bump helm chart version reference to 0.42.2 #19839

Closed

octavia-squidington-iii mentioned this pull request Nov 30, 2022

Bump Airbyte version from 0.40.22 to 0.40.23 #19941

Merged

tryangul mentioned this pull request Jun 28, 2023

Improve RetryWithJitter #26084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Retry Logic to Airbyte API calls. #19693

Add Retry Logic to Airbyte API calls. #19693

davinchia commented Nov 22, 2022 •

edited

Loading

davinchia commented Nov 24, 2022

davinchia Nov 24, 2022

davinchia Nov 24, 2022

davinchia Nov 24, 2022

davinchia commented Nov 24, 2022

xiaohansong left a comment

xiaohansong Nov 24, 2022

davinchia Nov 25, 2022 •

edited

Loading

Add Retry Logic to Airbyte API calls. #19693

Add Retry Logic to Airbyte API calls. #19693

Conversation

davinchia commented Nov 22, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

davinchia commented Nov 24, 2022

davinchia Nov 24, 2022

Choose a reason for hiding this comment

davinchia Nov 24, 2022

Choose a reason for hiding this comment

davinchia Nov 24, 2022

Choose a reason for hiding this comment

davinchia commented Nov 24, 2022

xiaohansong left a comment

Choose a reason for hiding this comment

xiaohansong Nov 24, 2022

Choose a reason for hiding this comment

davinchia Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

davinchia commented Nov 22, 2022 •

edited

Loading

davinchia Nov 25, 2022 •

edited

Loading