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

[Java CDK] Fix java interop compilation issue in Config/TransientErrorException. #41996

Merged
merged 13 commits into from
Jul 17, 2024
Merged

[Java CDK] Fix java interop compilation issue in Config/TransientErrorException. #41996

merged 13 commits into from
Jul 17, 2024

Conversation

akashkulk
Copy link
Contributor

@akashkulk akashkulk commented Jul 16, 2024 •
edited
Loading

Fixes a compilation issue in the construction of transient & config errors which was a minor regression introduced in a previous PR due to issues with Kotlin/Java interop

@vercel Vercel
Copy link

vercel bot commented Jul 16, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2024 2:01pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/source/mysql labels Jul 16, 2024
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jul 16, 2024
@akashkulk akashkulk marked this pull request as ready for review July 16, 2024 17:27
@akashkulk akashkulk requested review from a team as code owners July 16, 2024 17:27
@akashkulk akashkulk requested a review from edgao July 16, 2024 17:27
edgao
edgao approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

I only reviewed the exception constructor stuff, one nit in the changelog + a nonblocking suggestion

airbyte-cdk/java/airbyte-cdk/README.md Outdated Show resolved Hide resolved
...rbyte-cdk/dependencies/src/main/kotlin/io/airbyte/commons/exceptions/ConfigErrorException.kt Outdated Show resolved Hide resolved
@theyueli
Copy link
Contributor

  1. Fixes a compilation issue in the construction of transient & config errors which was a minor regression introduced in a previous PR due to issues with Kotlin/Java interop

Do we know why this regression was not caught by any of the tests done by CI when we merged the last one?

@evantahler
Copy link
Contributor

Possibly related to #41999

theyueli
theyueli reviewed Jul 16, 2024
View reviewed changes
...in/java/io/airbyte/integrations/source/mysql/initialsync/MySqlInitialLoadRecordIterator.java Outdated
@@ -213,6 +213,11 @@ public void close() throws Exception {
}
}

@Override
public Optional<AirbyteStreamNameNamespacePair> getAirbyteStream() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the problem/ticket resulting from the lack of this implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default implementation of getAirbyteStream() is to return Optional.empty() so each iterator that implements the AirbyteStreamAware interface needs to implement this method.

I noticed this during testing that we were throwing a NoSuchElementException instead of the relevant TransientException

@akashkulk
Copy link
Contributor Author

  1. Fixes a compilation issue in the construction of transient & config errors which was a minor regression introduced in a previous PR due to issues with Kotlin/Java interop

Do we know why this regression was not caught by any of the tests done by CI when we merged the last one?

Mentioned this in the standup - connector tests are run only if the CDK version is togged. Likely for that PR only the relevant (destination) tests were run so this wasn't caught

@akashkulk akashkulk requested a review from theyueli July 16, 2024 19:39
theyueli
theyueli approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@theyueli theyueli left a comment

Choose a reason for hiding this comment

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

Looks good, let's split into different smaller PRs next time (as they are addressing issues that are orthogonal) :-)

rodireich
rodireich reviewed Jul 16, 2024
View reviewed changes
...in/java/io/airbyte/integrations/source/mysql/initialsync/MySqlInitialLoadRecordIterator.java Outdated
@@ -213,6 +213,11 @@ public void close() throws Exception {
}
}

@Override
public Optional<AirbyteStreamNameNamespacePair> getAirbyteStream() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a similar change required on other connectors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That will have been reflected in the PRs associated with WASS changes in the relevant connectors

rodireich
rodireich reviewed Jul 16, 2024
View reviewed changes
...ces/src/main/kotlin/io/airbyte/cdk/integrations/debezium/internals/DebeziumRecordIterator.kt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that due to #41647 DebeziumRecordIterator.isHeartbeatEvent() is currently broken.
Not sure if limited to a driver or a version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this PR wont help that case. Then again it won't handle or make anything worse.

@octavia-squidington-iii octavia-squidington-iii removed area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Jul 17, 2024
@akashkulk akashkulk changed the title [DB Sources] Add back heartbeat timeout [Java CDK] Fix java interop compilation issue in Config/TransientErrorException. Jul 17, 2024
@akashkulk
Copy link
Contributor Author

akashkulk commented Jul 17, 2024 •
edited by github-actions bot
Loading

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/9975713155
✅ Successfully published Java CDK version=0.41.6!

@akashkulk akashkulk merged commit 990d1e3 into master Jul 17, 2024
33 checks passed
@akashkulk akashkulk deleted the akash/hb-add branch July 17, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theyueli theyueli theyueli approved these changes

@edgao edgao edgao approved these changes

@rodireich rodireich rodireich approved these changes

Assignees
No one assigned
Labels
CDK Connector Development Kit connectors/source/mysql
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@akashkulk @theyueli @evantahler @rodireich @edgao @octavia-squidington-iii