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

fix kotlin warnings in destination CDK submodules #37481

Merged
merged 1 commit into from Apr 23, 2024
Merged

fix kotlin warnings in destination CDK submodules #37481

merged 1 commit into from Apr 23, 2024

Conversation

stephane-airbyte
Copy link
Contributor

@stephane-airbyte stephane-airbyte commented Apr 22, 2024 •
edited

cleaning up kotlin warnings

@vercel Vercel
Copy link

vercel bot commented Apr 22, 2024 •
edited

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 Apr 22, 2024 10:51pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Apr 22, 2024
@stephane-airbyte Graphite App
Copy link
Contributor Author

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

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

@stephane-airbyte stephane-airbyte marked this pull request as ready for review April 22, 2024 19:13
@stephane-airbyte stephane-airbyte requested a review from a team as a code owner April 22, 2024 19:13
@stephane-airbyte stephane-airbyte requested review from edgao, gisripa and postamar and removed request for a team April 22, 2024 19:13
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-22-fix_kotlin_warnings_in_destination_cdk_submodules branch from 6ee805a to fc7b7e9 Compare April 22, 2024 19:42
edgao
edgao approved these changes Apr 22, 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.

took a quick skim over this, didn't go into depth on e.g. the removed method params. Had a couple specific questions but lgtm otherwise

...ions/src/main/kotlin/io/airbyte/cdk/integrations/destination/jdbc/JdbcInsertFlushFunction.kt Outdated
@@ -15,9 +15,9 @@ class JdbcInsertFlushFunction(
override val optimalBatchSizeBytes: Long
) : DestinationFlushFunction {
@Throws(Exception::class)
override fun flush(desc: StreamDescriptor, stream: Stream<PartialAirbyteMessage>) {
override fun flush(decs: StreamDescriptor, stream: Stream<PartialAirbyteMessage>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this to match super declaration? I'd prefer to modify the base class to use desc, since presumably this is shorthand for "descriptor"

...-destinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/staging/SerialFlush.kt
pair.name,
FileUtils.byteCountToDisplaySize(writer.byteCount)
)
log.info {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.info {
log.info {"Flushing buffer for stream ${pair.name} (${FileUtils.byteCountToDisplaySize(writer.byteCount)}) to staging"}

not actually sure I understand what's written right now - why is there "${"..."}"? (similar question for the other log.error call)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://kotlinlang.org/docs/strings.html#string-templates

...ing-deduping/src/main/kotlin/io/airbyte/integrations/base/destination/typing_deduping/Sql.kt
}
.map { statement: String ->
.filter { statement: String? -> !statement.isNullOrEmpty() }
.map internalMap@{ statement: String ->
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 internalMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://kotlinlang.org/docs/returns.html#return-to-labels

@stephane-airbyte stephane-airbyte force-pushed the stephane/04-22-fix_kotlin_warnings_in_destination_cdk_submodules branch from fc7b7e9 to a393292 Compare April 22, 2024 22:32
@stephane-airbyte stephane-airbyte requested a review from a team as a code owner April 22, 2024 22:32
@stephane-airbyte stephane-airbyte force-pushed the stephane/04-22-fix_kotlin_warnings_in_destination_cdk_submodules branch from a393292 to 1696178 Compare April 22, 2024 22:50
postamar
postamar reviewed Apr 23, 2024
View reviewed changes
...stinations/src/main/kotlin/io/airbyte/cdk/integrations/destination/jdbc/JdbcSqlOperations.kt
@@ -23,7 +23,7 @@ import org.apache.commons.csv.CSVPrinter
abstract class JdbcSqlOperations : SqlOperations {
protected val schemaSet: MutableSet<String?> = HashSet()

protected constructor() {}
protected constructor()
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking question: curious what's the point of a no-arg no-op protected constructor in an abstract class?

postamar
postamar reviewed Apr 23, 2024
View reviewed changes
...ons/src/main/kotlin/io/airbyte/cdk/integrations/destination/jdbc/copy/gcs/GcsStreamCopier.kt
csvPrinters[gcsFileName]!!.printRecord(
override fun write(id: UUID?, recordMessage: AirbyteRecordMessage?, fileName: String?) {
if (csvPrinters.containsKey(fileName)) {
csvPrinters[fileName]!!.printRecord(
Copy link
Contributor

Choose a reason for hiding this comment

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

csvPrinters[fileName]?.let { ... }

postamar
postamar reviewed Apr 23, 2024
View reviewed changes
...ng/src/main/kotlin/io/airbyte/integrations/base/destination/typing_deduping/CatalogParser.kt
when (schema) {
when (
val schema: AirbyteType =
AirbyteType.Companion.fromJsonSchema(stream.stream.jsonSchema)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the Companion need to be referenced explicitly?

@stephane-airbyte stephane-airbyte merged commit 978142e into master Apr 23, 2024
35 checks passed
@stephane-airbyte stephane-airbyte deleted the stephane/04-22-fix_kotlin_warnings_in_destination_cdk_submodules branch April 23, 2024 14:02
strosek pushed a commit that referenced this pull request Apr 24, 2024
@stephane-airbyte @strosek
fix kotlin warnings in destination CDK submodules (#37481)
279eee9 
cleaning up kotlin warnings
FVidalCarneiro pushed a commit to AgiData/airbyte that referenced this pull request May 2, 2024
@stephane-airbyte @FVidalCarneiro
fix kotlin warnings in destination CDK submodules (airbytehq#37481)
ce88b04 
cleaning up kotlin warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@postamar postamar postamar approved these changes

@edgao edgao edgao approved these changes

@gisripa gisripa Awaiting requested review from gisripa

Assignees
No one assigned
Labels
CDK Connector Development Kit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stephane-airbyte @postamar @edgao @octavia-squidington-iii