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.

Sign up for GitHub

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

Jump to bottom

CDC Postgres Tests #2777

Merged
merged 3 commits into from
Apr 7, 2021
Merged

CDC Postgres Tests #2777

merged 3 commits into from
Apr 7, 2021

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Apr 7, 2021

What

  • Adds tests. Fixes issues along the way.

How

Describe the solution

Pre-merge Checklist

  • Figure out breaking behavior for deletions
  • Fix helper to assert one state message
  • Add test that check AirbyteRecordMessage metadata fields.

Recommended reading order

  1. PostgresSourceCdcTest.java
  2. DebeziumRecordPublisherTest.java
  3. the rest

@cgardens cgardens removed the request for review from ChristopheDuong April 7, 2021 01:02
cgardens
cgardens commented Apr 7, 2021
View reviewed changes
...-postgres/src/main/java/io/airbyte/integrations/source/postgres/DebeziumRecordPublisher.java Outdated
@@ -121,7 +123,7 @@ protected static Properties getDebeziumProperties(JsonNode config, ConfiguredAir
props.setProperty("value.converter.schemas.enable", "false");

// https://debezium.io/documentation/reference/configuration/event-flattening.html
props.setProperty("delete.handling.mode", "rewrite");
// props.setProperty("delete.handling.mode", "rewrite");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

work in progress on this property.

@cgardens cgardens requested a review from davinchia April 7, 2021 01:03
michel-tricot
michel-tricot reviewed Apr 7, 2021
View reviewed changes
airbyte-commons/src/main/java/io/airbyte/commons/util/AutoCloseableIterators.java Outdated
@@ -72,6 +72,12 @@
return new DefaultAutoCloseableIterator<>(stream.iterator(), stream::close);
}

public static <T> List<T> toList(AutoCloseableIterator<T> iterator) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment to expose that it closes the iterator?

Copy link
Contributor

Choose a reason for hiding this comment

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

might be clearer to name this function something like toListAndClose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

...e-postgres/src/main/java/io/airbyte/integrations/source/postgres/DebeziumRecordIterator.java Outdated
@@ -148,7 +148,10 @@ private PgLsn extractLsn(ChangeEvent<String, String> event) {
.flatMap(source -> Optional.ofNullable(source.get("lsn").asText()))
.map(Long::parseLong)
.map(PgLsn::fromLong)
.orElseThrow(() -> new IllegalStateException("Could not find LSN"));
.orElseThrow(() -> {
System.out.println("event = " + event);
Copy link
Contributor

Choose a reason for hiding this comment

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

debug statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! still debugging this bit with the delete issue i mentioned.

davinchia
davinchia reviewed Apr 7, 2021
View reviewed changes
airbyte-integrations/connectors/source-appsflyer-singer/unit_tests/unit_test.py Outdated
@@ -22,6 +22,5 @@
SOFTWARE.
"""


def test_example_method():
Copy link
Contributor

Choose a reason for hiding this comment

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

accidental?

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 think these are the actual correct style? will make sure to run style check again.

davinchia
davinchia approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense! Briefly looked through to general flow. I did not examine the test cases in detail as I assume you and Jared have the spectrum covered. Let me know if you wanted a specific focus on an area.

One thoughts on testing. What do you think about using the @DisplayName annotation with Gherkin verbs (Given, When, Should etc) to better summarise what a test is doing? e.g. with the testWhitelistFiltersFullRefresh the display name can read: "When reading multiple streams should only return whitelist for incremental streams". Usage is up to dev discretion. Although there is a small cost, I find it really saves time with understanding tests, especially for beefier tests. e.g. testRecordsProducedDuringAndAfterSync, since the reader doesn't need to parse everything in the test block. Just a thought; not blocking.

@cgardens cgardens merged commit a2a979b into jrhizor/debezium Apr 7, 2021
@cgardens cgardens deleted the cgardens/cdc_tests branch April 7, 2021 19:02
jrhizor added a commit that referenced this pull request Apr 9, 2021
@jrhizor @cgardens
postgres cdc (#2548)
2b19da8 
* spike

* more

* debezium wip

* use oneof for configuration

* iterator wrapping structure

* push current

* working loop

* move capability into source

* hack it into a sharable state

* debezium test runner (#2617)

* CDC Wait for Values (#2618)

* output actual AirbyteMessages for cdc (#2631)

* message conversion

* fmt

* add lsn extraction and comparison (#2613)

* postgres cdc catalog (#2673)

* update cdc catalog

* A

* table selection for cdc (#2690)

* table selection for cdc

* fix broken merge

* also test double quote in name

* Add state management to CDC (#2718)

* CDC: Fix Producer/Consumer State Machine (#2721)

* CDC Postgres Tests (#2777)

* fix postgres cdc image name and run check before reading data (#2785)

* minor postgres cdc fixes

* add test and fix check behavior

* fix

* improve comment

* remove unused props, remove todos, add some more sanity tests (#2791)

* cdc: add offset store tests (#2793)

* clean (#2798)

* postgres cdc docs (#2784)

* cdc docs

* Update docs/integrations/sources/postgres.md

Co-authored-by: Charles <[email protected]>

* address gcp

* learn too english

* add link

* add more disk space warnings

* add additional cdc use case

* add information on how to find postgresql.conf

* add how to find the file

Co-authored-by: Charles <[email protected]>

* various merge conflict fixes (#2799)

* cdc standard tests (#2813)

* require cdc users to create publications & update docs (#2818)

* postgres cdc race condition

* working? but different process

* add additional logging to help debug in the future

* everything done except working config

* remove unintended change

* Use oneOf in PG CDC spec (#2827)

* add oneOf configuration for postgres cdc  (#2831)

* add oneof configuration for cdc postgres

* fmt

Co-authored-by: Charles <[email protected]>

* fix test (#2834)

* fix test

* bump version

* add docs on creating replica identities (#2838)

* add docs on creating replica identities

* emphasize danger

* grammar

* bump pg version in source catalog

* generate seed files

Co-authored-by: cgardens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michel-tricot michel-tricot michel-tricot left review comments

@jrhizor jrhizor jrhizor approved these changes

@davinchia davinchia davinchia approved these changes

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
@cgardens @jrhizor @michel-tricot @davinchia