debuggability improvements to the CDK #37746

stephane-airbyte · 2024-05-01T21:06:48Z

A few changes to improve debuggability:

We now print the stack trace that created an orphan thread, in addition to the current stack trace.
we add the ability for a connector to tell the integration runner to not care about certain threads (by adding a thread filter)
We change the log of connector containers to include the name of the test that created it (usually each test will create and close a container)
We also log when the timeout timer gets triggered, to try to understand why some tests are still taking way more time than their timeout should allow

Another change that was initially in there but has been removed and might be worth thinking about:
Today, only Write checks for live threads on shutdown. We might want to do that for other operations as well

vercel · 2024-05-01T21:06:55Z

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		May 2, 2024 9:09pm

stephane-airbyte · 2024-05-01T21:07:09Z

minor changes for destination-snowflake #37695
convert destination-snowflake to Kotlin CDK #36910
debuggability improvements to the CDK #37746 👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @stephane-airbyte and the rest of your teammates on Graphite

edgao

couple nitpicks and some questions to make sure I understand what's going on. Slightly spooky PR but I don't see a better way to do it so 🤷

.../java/airbyte-cdk/core/src/main/kotlin/io/airbyte/cdk/integrations/base/IntegrationRunner.kt

...res/kotlin/io/airbyte/cdk/integrations/standardtest/destination/DestinationAcceptanceTest.kt

...e-cdk/core/src/testFixtures/kotlin/io/airbyte/cdk/extensions/LoggingInvocationInterceptor.kt

postamar · 2024-05-02T18:45:52Z

...e-cdk/core/src/testFixtures/kotlin/io/airbyte/cdk/extensions/LoggingInvocationInterceptor.kt

                        "Junit starting {} with timeout of {}",
                        logLineSuffix,
                        DurationFormatUtils.formatDurationWords(timeout.toMillis(), true, true)
                    )
-                    Timer("TimeoutTimer-" + currentThread.name, true)
-                        .schedule(timeoutTask, timeout.toMillis())
+                    var timer =


is this var referenced anywhere? doesn't seem to be the case

postamar · 2024-05-02T18:47:02Z

...e-cdk/java/airbyte-cdk/core/src/testFixtures/kotlin/io/airbyte/cdk/extensions/TestContext.kt

+
+class TestContext {
+    companion object {
+        val CURRENT_TEST_NAME: ThreadLocal<String?> = ThreadLocal()


Convention in kotlin is to reserve upper-case snake case to const vals. I'm surprised your IDE didn't hint about this.

from the kotlin guide:
Names of constants (properties marked with const, or top-level or object val properties with no custom get function that hold deeply immutable data) should use uppercase underscore-separated (screaming snake case) names

I think this is what we're supposed to do

TIL! Thanks

postamar · 2024-05-02T18:52:10Z

build.gradle

@@ -139,6 +139,9 @@ allprojects {
        // This is also required, to prevent stderr spam starting with
        //   "OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader cl..."
        jvmArgs "-Xshare:off"
+        // This is required for our dangling thread checks at the end of a connector run.
+        // see IntegrationRunner.stopOrphanedThreads


I looked at this function and I'm none the wiser as to why this --add-opens flag is required. This might be out of scope of this PR but if you do know why please take this opportunity to add some more detailed explanations.

it's because we use introspection to change the access of the ThreadLocal.get(Thread)

postamar · 2024-05-02T18:56:42Z

.../java/airbyte-cdk/core/src/main/kotlin/io/airbyte/cdk/integrations/base/IntegrationRunner.kt

+            ThreadLocal::class.java.getDeclaredMethod("get", Thread::class.java)
+        init {
+            getMethod.isAccessible = true
+        }


fyi you can also do

private val getMethod: Method = ThreadLocal::class.java.getDeclaredMethod("get", Thread::class.java).also { isAccessible = true }

postamar

I'm happy with the changes in IntegrationRunner so this is good to go as far as I'm concerned. If it works, it works.

stephane-airbyte · 2024-05-03T06:05:12Z

/publish-java-cdk

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

mtrienis-dw-v2 · 2024-05-14T00:11:43Z

.../java/airbyte-cdk/core/src/main/kotlin/io/airbyte/cdk/integrations/base/IntegrationRunner.kt

+        // opened method
+        private val getMethod: Method =
+            ThreadLocal::class.java.getDeclaredMethod("get", Thread::class.java).also {
+                it.isAccessible = true


I'm getting an error here, not sure if I'm doing something silly or if there was a bug introduced here?

@stephane-airbyte

Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make private java.lang.Object java.lang.ThreadLocal.get(java.lang.Thread) accessible: module java.base does not "opens java.lang" to unnamed module @5f058f00

I'm not sure how you're executing this. I've tested this when running tests from intelliJ and airbyte-ci, as well as when running a connector through the docker image

@stephane-airbyte try running this main method through Intellij IDE:

https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/destination-mysql/src/main/java/io/airbyte/integrations/destination/mysql/MySQLDestination.java#L188-L193

With the following parameters:

Java 21 SDK

Cli arguments --write --config /<my_path>/config.json --catalog /<my_path>/configured_catalog.json

Click Modify options -> Redirect input and then add example input message <my_path>/message.json

you need an additional JVM parameters, --add-opens=java.base/java.lang=ALL-UNNAMED

Roger -- thanks!

octavia-squidington-iii added the CDK Connector Development Kit label May 1, 2024

This was referenced May 1, 2024

convert destination-snowflake to Kotlin CDK #36910

Merged

minor changes for destination-snowflake #37695

Closed

stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch 2 times, most recently from 5b62126 to 997351f Compare May 1, 2024 22:24

edgao approved these changes May 2, 2024

View reviewed changes

postamar reviewed May 2, 2024

View reviewed changes

postamar approved these changes May 2, 2024

View reviewed changes

gisripa approved these changes May 2, 2024

View reviewed changes

stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch 5 times, most recently from 961d513 to 55bf52e Compare May 2, 2024 20:47

debuggability improvements to the CDK

41beb7a

stephane-airbyte force-pushed the stephane/05-01-debuggability_improvements_to_the_cdk branch from 55bf52e to 41beb7a Compare May 2, 2024 21:09

stephane-airbyte marked this pull request as ready for review May 3, 2024 06:01

stephane-airbyte requested review from a team as code owners May 3, 2024 06:01

stephane-airbyte merged commit c326286 into master May 3, 2024
32 checks passed

stephane-airbyte deleted the stephane/05-01-debuggability_improvements_to_the_cdk branch May 3, 2024 13:50

mtrienis-dw-v2 reviewed May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debuggability improvements to the CDK #37746

debuggability improvements to the CDK #37746

stephane-airbyte commented May 1, 2024 •

edited

Loading

vercel bot commented May 1, 2024 •

edited

Loading

stephane-airbyte commented May 1, 2024 •

edited

Loading

edgao left a comment

postamar May 2, 2024

postamar May 2, 2024

stephane-airbyte May 2, 2024

postamar May 3, 2024

postamar May 2, 2024

stephane-airbyte May 2, 2024

postamar May 2, 2024

postamar left a comment

stephane-airbyte commented May 3, 2024 •

edited by github-actions bot

Loading

mtrienis-dw-v2 May 14, 2024 •

edited

Loading

stephane-airbyte May 14, 2024

mtrienis-dw-v2 May 14, 2024

stephane-airbyte May 14, 2024

mtrienis-dw-v2 May 14, 2024

debuggability improvements to the CDK #37746

debuggability improvements to the CDK #37746

Conversation

stephane-airbyte commented May 1, 2024 • edited Loading

vercel bot commented May 1, 2024 • edited Loading

stephane-airbyte commented May 1, 2024 • edited Loading

edgao 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

postamar left a comment

Choose a reason for hiding this comment

stephane-airbyte commented May 3, 2024 • edited by github-actions bot Loading

mtrienis-dw-v2 May 14, 2024 • edited Loading

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

stephane-airbyte commented May 1, 2024 •

edited

Loading

vercel bot commented May 1, 2024 •

edited

Loading

stephane-airbyte commented May 1, 2024 •

edited

Loading

stephane-airbyte commented May 3, 2024 •

edited by github-actions bot

Loading

mtrienis-dw-v2 May 14, 2024 •

edited

Loading