Checkpointing Testing #3473

cgardens · 2021-05-18T23:00:59Z

What

Testing Checkpointing (🎉 Checkpointing: Worker use destination (instead of source) for state #3290) required a lot of extra pieces, so splitting it into its own PR so it's easier to consume

How

Create a "test" source and destination. These allow us to just define really test specific functionality freely. This is good for testing important edge cases.
In this case we use ExceptionAfterNSource and LoggingDestination so that we could write an acceptance test for checkpointing. The strategy is that the source emits N records and then throws an exception. Then we verify that the state afterwards is still retained.
This required exposing the state somehow. I chose to throw it in the API, since that seems to be something that we're going to do soon anyway and was pretty straight forward.

Pre-merge Checklist

~~Add back pressure test that uses ThrottledDestination and InfiniteFeedSource~~(punting this: Add back pressure test into src #3596)

Recommended reading order

E2ETestSource.java
E2ETestDestination.java
config.yaml
the rest

┆Issue is synchronized with this Asana task by Unito

michel-tricot

Terrific! Great idea to encode behaviors in testing sources & destinations

michel-tricot · 2021-05-19T01:41:12Z

...-e2e-test/src/main/java/io/airbyte/integrations/destination/e2e_test/E2ETestDestination.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class E2ETestDestination extends BaseConnector implements Destination {


Except for the name, this is brilliant!

What about End2EndTestDestination or TestingDestination?

michel-tricot · 2021-05-19T01:41:54Z

airbyte-integrations/connectors/destination-e2e-test/src/main/resources/spec.json

+ }
+ ]
+ }
+}


NIT: newline

michel-tricot · 2021-05-19T01:43:16Z

...ors/source-e2e-test/src/main/java/io/airbyte/integrations/source/e2e_test/E2ETestSource.java

+/**
+ * This source is designed to be a switch statement for our suite of highly-specific test sourcess.
+ */
+public class E2ETestSource extends BaseConnector implements Source {


TestingSources

davinchia

Looks great! Feel free to merge after dealing with the names.

One clarification: the unused test source & destination are checked in for convenience right? (I'm just making sure I didn't miss any other tests that were using them)

davinchia · 2021-05-21T07:34:49Z

airbyte-api/src/main/openapi/config.yaml

 description: Connection not found
 "422":
 $ref: "#/components/responses/InvalidInput"
+ /v1/connections/get_state:


This is a good idea - people have been asking for this.

I think they asked about /v1/connections/set_state: or /v1/connections/update_state: too, right?

definitely! i'm looking for directionally correct here. I know this doesn't fully hit the edit state use case.

shoudln't it be:
/v1/connections/state/get
or
/v1/state/get

davinchia · 2021-05-21T07:37:19Z

airbyte-integrations/connectors/destination-e2e-test/Dockerfile

+
+RUN tar xf ${APPLICATION}.tar --strip-components=1
+
+LABEL io.airbyte.version=0.3.1


good call. what if we go with dev here actually? not planning to publish this one.

davinchia · 2021-05-21T07:37:58Z

airbyte-integrations/connectors/destination-e2e-test/build.gradle

+
+ integrationTestJavaImplementation "org.testcontainers:postgresql:1.15.1"
+
+ implementation files(project(':airbyte-integrations:bases:base-java').airbyteDocker.outputs)


nit: move this out of the test dependencies

why? the dockerfile still depends on that base image, no?

davinchia · 2021-05-21T07:38:47Z

airbyte-integrations/connectors/source-e2e-test/build.gradle

+
+ integrationTestJavaImplementation project(':airbyte-integrations:bases:standard-source-test')
+
+ implementation files(project(':airbyte-integrations:bases:base-java').airbyteDocker.outputs)


nit: move this out of test dependencies.

davinchia · 2021-05-21T08:06:34Z

...ource-e2e-test/src/main/java/io/airbyte/integrations/source/e2e_test/InfiniteFeedSource.java

+ * SOFTWARE.
+ */
+
+package io.airbyte.integrations.source.e2e_test;


haha I wrote the same version in Python!

davinchia · 2021-05-21T08:18:48Z

...2e-test/src/test/java/io/airbyte/integrations/source/e2e_test/ExceptionAfterNSourceTest.java

+ assertEquals(getRecordMessage(8L).getRecord().getData(), read.next().getRecord().getData());
+ assertEquals(getRecordMessage(9L).getRecord().getData(), read.next().getRecord().getData());
+ assertEquals(getRecordMessage(10L).getRecord().getData(), read.next().getRecord().getData());
+ assertEquals(getStateMessage(10L).getState().getData(), read.next().getState().getData());


is it worth checking for an exception here?

yes. good call!

michel-tricot · 2021-05-21T19:00:25Z

airbyte-api/src/main/openapi/config.yaml

 description: Connection not found
 "422":
 $ref: "#/components/responses/InvalidInput"
+ /v1/connections/get_state:


shoudln't it be:
/v1/connections/state/get
or
/v1/state/get

…3290) * Migrate BufferedStreamConsumer users (e.g. all JDBC destinations, MeiliSearch) (#3473) * Add checkpointing test cases in Acceptance Tests (#3473) * Add testing for emitting state in Destination Standard Test (#3546) * Migrate BQ to support checkpointing (#3546) * Migrate copy destinations support checkpointing (#3547) * Checkpointing: Migrate CSV and JSON destinations (#3551)

cgardens requested review from jrhizor and masonwheeler May 18, 2021 23:01

cgardens mentioned this pull request May 18, 2021

🎉 Checkpointing: Worker use destination (instead of source) for state #3290

Merged

4 tasks

michel-tricot reviewed May 19, 2021

View reviewed changes

cgardens added 2 commits May 20, 2021 22:35

test checkpointing

7a7ace0

add dep on source tests

1624e10

cgardens force-pushed the cgardens/checkpointing_testing2 branch from b2c9fb8 to c822c7b Compare May 21, 2021 05:36

add more logging to tets

4de1bc7

cgardens force-pushed the cgardens/checkpointing_testing2 branch from c822c7b to 4de1bc7 Compare May 21, 2021 05:36

cgardens force-pushed the cgardens/checkpointing_respect_destination_state branch from dba87c8 to ae161d7 Compare May 21, 2021 05:36

davinchia approved these changes May 21, 2021

View reviewed changes

ChristopheDuong approved these changes May 21, 2021

View reviewed changes

tuliren approved these changes May 21, 2021

View reviewed changes

ChristopheDuong mentioned this pull request May 21, 2021

New destination: AssertDestination for test #3534

Closed

michel-tricot approved these changes May 21, 2021

View reviewed changes

code review feedback

b570a0a

cgardens force-pushed the cgardens/checkpointing_respect_destination_state branch from aa941af to 81cce57 Compare May 22, 2021 20:58

=> state resource

d4e2df1

cgardens merged commit 16937ec into cgardens/checkpointing_respect_destination_state May 25, 2021

cgardens deleted the cgardens/checkpointing_testing2 branch May 25, 2021 20:44

cgardens added a commit that referenced this pull request May 25, 2021

Checkpointing Testing (#3473)

9075c21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Checkpointing Testing #3473

Checkpointing Testing #3473

Uh oh!

cgardens commented May 18, 2021 •

edited

Loading

michel-tricot left a comment

michel-tricot May 19, 2021

michel-tricot May 19, 2021

michel-tricot May 19, 2021

davinchia left a comment

davinchia May 21, 2021

ChristopheDuong May 21, 2021

cgardens May 21, 2021

michel-tricot May 21, 2021

davinchia May 21, 2021

cgardens May 21, 2021

davinchia May 21, 2021

cgardens May 21, 2021

davinchia May 21, 2021

cgardens May 21, 2021

davinchia May 21, 2021

davinchia May 21, 2021

cgardens May 21, 2021

michel-tricot May 21, 2021

Labels

6 participants

+  }
+  ]
+  }
+ }    No newline at end of file


		RUN tar xf ${APPLICATION}.tar --strip-components=1

		LABEL io.airbyte.version=0.3.1


		integrationTestJavaImplementation "org.testcontainers:postgresql:1.15.1"

		implementation files(project(':airbyte-integrations:bases:base-java').airbyteDocker.outputs)


		integrationTestJavaImplementation project(':airbyte-integrations:bases:standard-source-test')

		implementation files(project(':airbyte-integrations:bases:base-java').airbyteDocker.outputs)

Checkpointing Testing #3473

Checkpointing Testing #3473

Uh oh!

Conversation

cgardens commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Pre-merge Checklist

Recommended reading order

michel-tricot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

6 participants

cgardens commented May 18, 2021 •

edited

Loading