Skip to content

KAFKA-15248 Add BooleanConverter#14093

Merged
yashmayya merged 10 commits intoapache:trunkfrom
hgeraldino:booleanconverter
Sep 26, 2023
Merged

KAFKA-15248 Add BooleanConverter#14093
yashmayya merged 10 commits intoapache:trunkfrom
hgeraldino:booleanconverter

Conversation

@hgeraldino
Copy link
Copy Markdown
Contributor

JIRA: KAFKA-15248
KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-959%3A+Add+BooleanConverter+to+Kafka+Connect

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@hgeraldino hgeraldino marked this pull request as ready for review July 25, 2023 18:22
@gharris1727 gharris1727 added connect kip Requires or implements a KIP labels Jul 26, 2023
Copy link
Copy Markdown
Contributor

@yashmayya yashmayya left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino, I just had a couple of minor comments.

@hgeraldino
Copy link
Copy Markdown
Contributor Author

Thanks @yashmayya for your review!

I addressed your comments, please take another look when you have a chance

Copy link
Copy Markdown
Contributor

@yashmayya yashmayya left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino, LGTM! I just had a few more minor suggestions.

@hgeraldino
Copy link
Copy Markdown
Contributor Author

Thanks @hgeraldino, LGTM! I just had a few more minor suggestions.

Addressed the second round of comments. Thanks again!

@hgeraldino hgeraldino requested a review from yashmayya August 16, 2023 19:38
Copy link
Copy Markdown
Contributor

@vamossagar12 vamossagar12 left a comment

Choose a reason for hiding this comment

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

I just have a small question. Other than that LGTM

@yashmayya
Copy link
Copy Markdown
Contributor

@hgeraldino the Connect integration tests are failing with errors like:

org.apache.kafka.connect.errors.ConnectException: One or more plugins are missing ServiceLoader manifests may not be usable with plugin.discovery=service_load: [
classpath	org.apache.kafka.connect.converters.BooleanConverter	converter	undefined
classpath	org.apache.kafka.connect.converters.BooleanConverter	header_converter	undefined
]
Read the documentation at https://kafka.apache.org/documentation.html#connect_plugindiscovery for instructions on migrating your plugins to take advantage of the performance improvements of service_load mode. To silence this error, set plugin.discovery=hybrid_warn in the worker config.

This is related to KIP-898 and specifically changes that were made in #14055. Could you please update the Converter and HeaderConverter provider-configuration files to include the new BooleanConverter?

@hgeraldino
Copy link
Copy Markdown
Contributor Author

@hgeraldino the Connect integration tests are failing with errors like:

Thanks @yashmayya, adding entries for the new converter to the metadata fixed the build issues 👍

Copy link
Copy Markdown
Contributor

@yashmayya yashmayya left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @hgeraldino, the build looks good now. My apologies for not noticing these earlier, but I just had a couple more minor comments. This should be good to be merged once they're addressed. Thanks for your continued patience!

@hgeraldino
Copy link
Copy Markdown
Contributor Author

Thanks for the updates @hgeraldino, the build looks good now. My apologies for not noticing these earlier, but I just had a couple more minor comments. This should be good to be merged once they're addressed. Thanks for your continued patience!

Addressed both comments, good catch!

Copy link
Copy Markdown
Contributor

@yashmayya yashmayya left a comment

Choose a reason for hiding this comment

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

Thanks @hgeraldino, LGTM!

@yashmayya
Copy link
Copy Markdown
Contributor

The test failures are all unrelated and pass on rerun locally:

[Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest.testSyncTopicConfigs()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14093/15/testReport/junit/org.apache.kafka.connect.mirror.integration/MirrorConnectorsIntegrationTransactionsTest/Build___JDK_8_and_Scala_2_12___testSyncTopicConfigs__/)
[Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14093/15/testReport/junit/org.apache.kafka.connect.integration/ConnectorRestartApiIntegrationTest/Build___JDK_8_and_Scala_2_12___testMultiWorkerRestartOnlyConnector/)
[Build / JDK 17 and Scala 2.13 / kafka.server.DescribeClusterRequestTest.testDescribeClusterRequestExcludingClusterAuthorizedOperations(String).quorum=kraft](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14093/15/testReport/junit/kafka.server/DescribeClusterRequestTest/Build___JDK_17_and_Scala_2_13___testDescribeClusterRequestExcludingClusterAuthorizedOperations_String__quorum_kraft/)
[Build / JDK 17 and Scala 2.13 / org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14093/15/testReport/junit/org.apache.kafka.trogdor.coordinator/CoordinatorTest/Build___JDK_17_and_Scala_2_13___testTaskRequestWithOldStartMsGetsUpdated__/)
[Build / JDK 21 and Scala 2.13 / org.apache.kafka.tools.MetadataQuorumCommandTest.[1] Type=Raft-Combined, Name=testDescribeQuorumStatusSuccessful, MetadataVersion=3.7-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14093/15/testReport/junit/org.apache.kafka.tools/MetadataQuorumCommandTest/Build___JDK_21_and_Scala_2_13____1__Type_Raft_Combined__Name_testDescribeQuorumStatusSuccessful__MetadataVersion_3_7_IV0__Security_PLAINTEXT/)
[Build / JDK 11 and Scala 2.13 / org.apache.kafka.common.network.SslTransportLayerTest.[1] tlsProtocol=TLSv1.2, useInlinePem=false](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14093/15/testReport/junit/org.apache.kafka.common.network/SslTransportLayerTest/Build___JDK_11_and_Scala_2_13____1__tlsProtocol_TLSv1_2__useInlinePem_false/)
[Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.ConnectorRestartApiIntegrationTest.testMultiWorkerRestartOnlyConnector](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14093/15/testReport/junit/org.apache.kafka.connect.integration/ConnectorRestartApiIntegrationTest/Build___JDK_11_and_Scala_2_13___testMultiWorkerRestartOnlyConnector/)

Merging to trunk.

@yashmayya yashmayya merged commit 7107a75 into apache:trunk Sep 26, 2023
@hgeraldino hgeraldino deleted the booleanconverter branch September 29, 2023 00:39
rreddy-22 pushed a commit to rreddy-22/kafka-rreddy that referenced this pull request Oct 3, 2023
…14093)

Reviewers: Yash Mayya <yash.mayya@gmail.com>, Sagar Rao <sagarmeansocean@gmail.com>, Qichao Chu <5326144+ex172000@users.noreply.github.com>
k-wall pushed a commit to k-wall/kafka that referenced this pull request Nov 21, 2023
…14093)

Reviewers: Yash Mayya <yash.mayya@gmail.com>, Sagar Rao <sagarmeansocean@gmail.com>, Qichao Chu <5326144+ex172000@users.noreply.github.com>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
…14093)

Reviewers: Yash Mayya <yash.mayya@gmail.com>, Sagar Rao <sagarmeansocean@gmail.com>, Qichao Chu <5326144+ex172000@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

connect kip Requires or implements a KIP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants