CDC partial reset acceptance test #14551

lmossman · 2022-07-09T01:43:43Z

What

Add acceptance test for partial reset of a CDC postgres source.

This PR also adds a second table to the incremental CDC acceptance test since it was needed for the new test anyway.

How

Describe the solution

lmossman · 2022-07-09T01:52:43Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/CdcAcceptanceTests.java

+
+    // We do not check that the source and the dest are in sync here because removing a stream doesn't
+    // delete its data in the destination
+    assertGlobalStateContainsStreams(connectionId, List.of(idAndNameStreamDescriptor));


I based this test pretty closely on Benoit's test here: https://github.com/airbytehq/airbyte/pull/14406/files#diff-50c027dd934aeff3adcaa28a849ae7c845d18961bedd7a4441e8bd82cef517f9R1056

Stopped here for now. Adding back a table to the CDC source is a bit more complex than in the non-CDC case since some extra SQL is required, and it may require removing and re-creating some CDC-specific resources. If we think this is important to add, we can look into that more next week

I added another test that turns a stream off and back on to test the partial reset functionality around adding a stream, so I think that should cover that case and is less complicated than trying to add a table back with the correct CDC sql configurations

@lmossman if the challenge that you are facing in adding a new table in CDC is around publication then you can the publication creation logic in the init script postgres_init_cdc.sql to include all tables

CREATE PUBLICATION airbyte_publication FOR ALL TABLES

lmossman · 2022-07-11T21:55:33Z

@benmoriceau @jdpgrailsdev @subodh1810 This PR is ready for review now. I had to use my locally built dev postgres source for the last test I added, because it relies on the changes that subodh made to the postgres source which haven't been released yet (so I don't expect CI to pass for this PR yet, but the tests are passing locally!). But from the results of that test, it seems that CDC partial reset logic is working as expected, so we can probably move forward with releasing the postgres source with those changes.

But please lmk if there is anything else that we should be testing here that I missed!

subodh1810 · 2022-07-12T16:19:19Z

@lmossman can you pull in the latest master in your branch and re-run the tests without the dev image logic? We did publish a new version of postgres here which does have CDC changes

lmossman · 2022-07-12T17:03:25Z

@lmossman can you pull in the latest master in your branch and re-run the tests without the dev image logic? We did publish a new version of postgres here which does have CDC changes

Will do!

benmoriceau

LGTM

benmoriceau · 2022-07-14T22:39:39Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/CdcAcceptanceTests.java

+
+  @Test
+  public void testPartialResetFromStreamSelection() throws Exception {
+    LOGGER.info("Starting partial reset cdc test");


Nit: you can pass a TestInfo test param to access the test name and log it.

benmoriceau · 2022-07-14T22:41:05Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/CdcAcceptanceTests.java

+  }
+
+  @Test
+  public void testPartialResetFromStreamSelection() throws Exception {


I agree but the main issue here is that if we break down this test, we will need to duplicate the first sync as many times as there is tests. It is very time consuming.

benmoriceau · 2022-07-14T22:42:51Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/CdcAcceptanceTests.java

+    assertNull(state.getGlobalState());
+  }
+
+  private WebBackendConnectionUpdate getUpdateInput(final ConnectionRead connection, final AirbyteCatalog catalog, final OperationRead operation) {


I will need to factorize that in my review, will probably move it to the test harness

This reverts commit 164d7da05990268b09e315eb88ff297d3a9f52f4.

github-actions bot added area/platform issues related to the platform area/server labels Jul 9, 2022

lmossman changed the base branch from master to anne/remove-with-refreshed-catalog July 9, 2022 01:44

lmossman temporarily deployed to more-secrets July 9, 2022 01:45 Inactive

lmossman commented Jul 9, 2022

View reviewed changes

lmossman requested review from benmoriceau and jdpgrailsdev July 9, 2022 01:55

lmossman changed the title ~~Lmossman/cdc partial reset acceptance test~~ CDC partial reset acceptance test Jul 11, 2022

lmossman marked this pull request as ready for review July 11, 2022 21:51

lmossman requested a review from subodh1810 July 11, 2022 21:53

lmossman temporarily deployed to more-secrets July 11, 2022 21:54 Inactive

lmossman temporarily deployed to more-secrets July 11, 2022 23:22 Inactive

alovew force-pushed the anne/remove-with-refreshed-catalog branch from cb7780b to b24ae1c Compare July 11, 2022 23:38

lmossman force-pushed the lmossman/cdc-partial-reset-acceptance-test branch from def7f0c to 6e03cba Compare July 12, 2022 17:29

lmossman requested a review from a team as a code owner July 12, 2022 17:29

octavia-squidington-iii added the connectors/source/github label Jul 12, 2022

github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit kubernetes labels Jul 12, 2022

lmossman temporarily deployed to more-secrets July 12, 2022 17:32 Inactive

lmossman force-pushed the lmossman/cdc-partial-reset-acceptance-test branch from 6e03cba to befd390 Compare July 12, 2022 17:33

lmossman temporarily deployed to more-secrets July 12, 2022 17:36 Inactive

lmossman changed the base branch from anne/remove-with-refreshed-catalog to lmossman/update-endpoint July 12, 2022 17:36

github-actions bot removed area/connectors Connector related issues area/documentation Improvements or additions to documentation kubernetes labels Jul 12, 2022

benmoriceau approved these changes Jul 14, 2022

View reviewed changes

lmossman force-pushed the lmossman/cdc-partial-reset-acceptance-test branch from f04fc4e to ce315b5 Compare July 15, 2022 00:01

lmossman temporarily deployed to more-secrets July 15, 2022 00:04 Inactive

lmossman temporarily deployed to more-secrets July 15, 2022 01:25 Inactive

lmossman temporarily deployed to more-secrets July 15, 2022 01:27 Inactive

github-actions bot added the area/server label Jul 15, 2022

lmossman temporarily deployed to more-secrets July 15, 2022 18:04 Inactive

lmossman added 16 commits July 15, 2022 14:15

set per stream feature flag to true for testing

7d279ec

add a second table to cdc acceptance tests

3d8bbc1

add partial reset test

f18219f

format

40bce5f

add partial reset cdc tests

9d03150

test incremental after partial reset

628d50a

remove dev image from acceptance test

92f4fc3

fix flag and add comment

e23e220

Revert "set per stream feature flag to true for testing"

db4951b

This reverts commit 164d7da05990268b09e315eb88ff297d3a9f52f4.

set USE_STREAM_CAPABLE_STATE flag to true in acceptance test script

9adc47c

call new update endpoint

2745cf4

use methods in test harness instead

a8f2f87

remove comment

7e9daf0

format

2d6d3c7

fix state check in basic acceptance test

6972f9b

use test info for test name logging

f8fbefe

lmossman force-pushed the lmossman/cdc-partial-reset-acceptance-test branch from 0a6b1f1 to f8fbefe Compare July 15, 2022 21:18

github-actions bot removed the area/server label Jul 15, 2022

lmossman temporarily deployed to more-secrets July 15, 2022 21:20 Inactive

lmossman merged commit 6005a9a into master Jul 15, 2022

lmossman deleted the lmossman/cdc-partial-reset-acceptance-test branch July 15, 2022 23:03

ryankfu mentioned this pull request Jul 18, 2022

Test and release per-stream state handling in Postgres Source #14457

Closed

3 tasks

octavia-squidington-iii mentioned this pull request Jul 23, 2022

Bump Airbyte version from 0.39.37-alpha to 0.39.38-alpha #14976

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDC partial reset acceptance test #14551

CDC partial reset acceptance test #14551

lmossman commented Jul 9, 2022

lmossman Jul 9, 2022 •

edited

Loading

lmossman Jul 11, 2022

subodh1810 Jul 12, 2022

lmossman commented Jul 11, 2022 •

edited

Loading

subodh1810 commented Jul 12, 2022

lmossman commented Jul 12, 2022

benmoriceau left a comment

benmoriceau Jul 14, 2022

lmossman Jul 15, 2022

benmoriceau Jul 14, 2022

benmoriceau Jul 14, 2022

CDC partial reset acceptance test #14551

CDC partial reset acceptance test #14551

Conversation

lmossman commented Jul 9, 2022

What

How

lmossman Jul 9, 2022 • edited Loading

Choose a reason for hiding this comment

lmossman Jul 11, 2022

Choose a reason for hiding this comment

subodh1810 Jul 12, 2022

Choose a reason for hiding this comment

lmossman commented Jul 11, 2022 • edited Loading

subodh1810 commented Jul 12, 2022

lmossman commented Jul 12, 2022

benmoriceau left a comment

Choose a reason for hiding this comment

benmoriceau Jul 14, 2022

Choose a reason for hiding this comment

lmossman Jul 15, 2022

Choose a reason for hiding this comment

benmoriceau Jul 14, 2022

Choose a reason for hiding this comment

benmoriceau Jul 14, 2022

Choose a reason for hiding this comment

lmossman Jul 9, 2022 •

edited

Loading

lmossman commented Jul 11, 2022 •

edited

Loading