Skip to content

KAFKA-8855; Collect and Expose Client's Name and Version in the Brokers (KIP-511 Part 1)#7381

Merged
cmccabe merged 4 commits intoapache:trunkfrom
dajac:kafka-8855-collect-client-name-and-version-2
Oct 4, 2019
Merged

KAFKA-8855; Collect and Expose Client's Name and Version in the Brokers (KIP-511 Part 1)#7381
cmccabe merged 4 commits intoapache:trunkfrom
dajac:kafka-8855-collect-client-name-and-version-2

Conversation

@dajac
Copy link
Copy Markdown
Member

@dajac dajac commented Sep 24, 2019

This PR implements the first part of KIP-511, namely:

  • update the ApiVersionsRequest/Response to the auto-generated ones;
  • bump the versions of ApiVersionsRequest/Response;
  • add ClientSoftwareName and ClientSoftwareVersions in the ApiVersionsRequest;
  • add validation for those two fields and handle error in the client;
  • implement the proposed fail-back mechanism.

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

Comment thread clients/src/main/resources/common/message/ApiVersionsResponse.json Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsRequest.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/clients/ApiVersion.java
Comment thread clients/src/main/java/org/apache/kafka/clients/NetworkClient.java Outdated
Copy link
Copy Markdown
Member

@mumrah mumrah 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, a few small nits.

Comment thread clients/src/main/java/org/apache/kafka/clients/NodeApiVersions.java
Comment thread core/src/main/scala/kafka/server/KafkaApis.scala
Comment thread clients/src/main/java/org/apache/kafka/clients/NetworkClient.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/clients/NodeApiVersions.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsRequest.java Outdated
Comment thread clients/src/main/resources/common/message/ApiVersionsResponse.json Outdated
@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Oct 3, 2019

Thanks, @dajac . This looks pretty close to being ready to go. I left a few comments.

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Oct 3, 2019

@cmccabe Thanks for the review. I have addressed all your comments.

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Oct 3, 2019

LGTM

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Oct 3, 2019

retest this please

1 similar comment
@dajac
Copy link
Copy Markdown
Member Author

dajac commented Oct 4, 2019

retest this please

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Oct 4, 2019

Failing tests

JDK 8 and Scala 2.11

  • org.apache.kafka.connect.integration.ConnectWorkerIntegrationTest.testAddAndRemoveWorker
  • org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector
  • org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSinkConnector
  • kafka.api.SslAdminClientIntegrationTest.testCreateTopicsResponseMetadataAndConfig

JDK 11 and Scala 2.12

  • org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector
  • org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSinkConnector

JDK 11 and Scala 2.13

  • org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector
  • org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSinkConnector

They are unrelated to the changes made in this PR.

@dajac
Copy link
Copy Markdown
Member Author

dajac commented Oct 4, 2019

I ran tests locally and got another failing test:

  • ReassignPartitionsClusterTest. shouldPerformMultipleReassignmentOperationsOverVariousTopics

I wanted to confirm that the failing tests in CI are flaky ones.

@cmccabe
Copy link
Copy Markdown
Contributor

cmccabe commented Oct 4, 2019

Those tests pass for me locally. Committing

@cmccabe cmccabe merged commit f98013c into apache:trunk Oct 4, 2019
@dajac dajac deleted the kafka-8855-collect-client-name-and-version-2 branch August 11, 2020 06:49
@hxfghgh
Copy link
Copy Markdown

hxfghgh commented Jul 30, 2024

hi my friend,i don't know why you have to "add ClientSoftwareName and ClientSoftwareVersions in the ApiVersionsRequest;"。 This does not seem to be a mandatory field to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants