-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
[Part 1 ] : Hackday project to debug connections #33027
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
Coverage report for source-postgres
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may reasonably think I'm being needlessly annoying in this review. With respect to the narrow scope of this particular change I absolutely am and I'm sorry. My interest here is to question old habits in matters of java code style, which I strongly feel need to change. We need to care more about readability. This PR happens to be greenfield development so the discussion feels warranted on my end, unlike a change involving legacy code. cc @stephane-airbyte who, I think, feels the same way about this stuff.
Wrt the review of this change in particular, having part 2 up would help me understand part 1 better.
...-sources/src/testFixtures/java/io/airbyte/cdk/integrations/debug/AbstractSourceDebugger.java
Outdated
Show resolved
Hide resolved
...-sources/src/testFixtures/java/io/airbyte/cdk/integrations/debug/AbstractSourceDebugger.java
Outdated
Show resolved
Hide resolved
.../source-postgres/src/test/java/io/airbyte/integrations/source/postgres/PostgresDebugger.java
Outdated
Show resolved
Hide resolved
throw new RuntimeException("WARNING: config indicates that we are clearing the WAL log while reading data. This will mutate the WAL log" | ||
+ " associated with the source being debugged and is not advised."); | ||
} | ||
final JsonNode debugConfig = ((ObjectNode) originalConfig.deepCopy()).put("debug_mode", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we couldn't simply decree that debug_mode
be used/reserved in all connectors for this purpose? I have the feeling that this line + the isDebugMode
method are going to be copy-pasted a lot otherwise.
.../source-postgres/src/test/java/io/airbyte/integrations/source/postgres/PostgresDebugger.java
Outdated
Show resolved
Hide resolved
...-sources/src/testFixtures/java/io/airbyte/cdk/integrations/debug/AbstractSourceDebugger.java
Outdated
Show resolved
Hide resolved
...-sources/src/testFixtures/java/io/airbyte/cdk/integrations/debug/AbstractSourceDebugger.java
Outdated
Show resolved
Hide resolved
...-sources/src/testFixtures/java/io/airbyte/cdk/integrations/debug/AbstractSourceDebugger.java
Outdated
Show resolved
Hide resolved
...source-postgres/src/test/java/io/airbyte/integrations/source/postgres/PostgresUtilsTest.java
Outdated
Show resolved
Hide resolved
...source-postgres/src/test/java/io/airbyte/integrations/source/postgres/PostgresUtilsTest.java
Outdated
Show resolved
Hide resolved
Not at all - I appreciate the comments! I will go through these and address them. As far as Part 2 goes, that is mainly a script not related to these efforts. There is also a small follow-up which creates debuggers for MongoDb, MySql, MsSql based on the patterns established in this PR so rather part 1 will dictate how part 2 will look like. |
I ended up rewriting a large chunk of it. I incorparated a lot of feedback from @postamar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Add a section on how to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning on doing a detailed write up. For now, I'm just hoping to get this in
@@ -15,7 +15,7 @@ java { | |||
airbyteJavaConnector { | |||
cdkVersionRequired = '0.5.3' | |||
features = ['db-sources'] | |||
useLocalCdk = false | |||
useLocalCdk = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to change back 💁♂️
/publish-java-cdk
|
Thanks for applying the suggestions @akashkulk and sorry for not circling back but it's all good. Much appreciated! |
Co-authored-by: akashkulk <akashkulk@users.noreply.github.com>
Co-authored-by: akashkulk <akashkulk@users.noreply.github.com>
This is the Java part of the debug hack project. Specifically
DebugUtil
utility debug classNext steps :