Skip to content

MINOR: Improve the org.apache.kafka.common.protocol code#7344

Merged
cmccabe merged 4 commits intoapache:trunkfrom
cmccabe:schema_improvements
Sep 25, 2019
Merged

MINOR: Improve the org.apache.kafka.common.protocol code#7344
cmccabe merged 4 commits intoapache:trunkfrom
cmccabe:schema_improvements

Conversation

@cmccabe
Copy link
Copy Markdown
Contributor

@cmccabe cmccabe commented Sep 16, 2019

Add UUID to the list of types documented in Type#toHtml. This was overlooked in the change which added UUIDs as a supported KRPC type.

Type, Protocol, ArrayOf: use Type#isArray and Type#arrayElementType rather than typecasting to handle arrays. This is cleaner. It will also make it easier for us to add compact arrays (as specified by KIP-482) as a new array type distinct from the old array type.

Add MessageUtil#byteBufferToArray, as well as a test for it. We will need this for handling tagged fields of type "bytes".

Schema#Visitor: we don't need a separate function overload for visiting arrays. We can just call "visit(Type field)".

TestUUID.json: reformat the JSON file to match the others.

ProtocolSerializationTest: improve the error messages on failure. Check that each type has the name we expect it to have.

@cmccabe cmccabe force-pushed the schema_improvements branch from 25b2d0d to 530f308 Compare September 16, 2019 20:18
Add UUID to the list of types documented in Type#toHtml.  This was overlooked
in the change which added UUIDs as a supported KRPC type.

Type, Protocol, ArrayOf: use Type#isArray and Type#arrayElementType rather than
typecasting to handle arrays.  This is cleaner.  It will also make it easier
for us to add compact arrays (as specified by KIP-482) as a new array type
distinct from the old array type.

Add MessageUtil#byteBufferToArray, as well as a test for it.  We will need this
for handling tagged fields of type "bytes".

Schema#Visitor: we don't need a separate function overload for visiting arrays.
We can just call "visit(Type field)".

TestUUID.json: reformat the JSON file to match the others.

ProtocolSerializationTest: improve the error messages on failure.  Check that
each type has the name we expect it to have.
@cmccabe cmccabe force-pushed the schema_improvements branch from 530f308 to 48c6d31 Compare September 17, 2019 16:43
Copy link
Copy Markdown
Contributor

@soondenana soondenana left a comment

Choose a reason for hiding this comment

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

LGTM. A bunch of nice improvements.

Comment thread clients/src/main/java/org/apache/kafka/common/protocol/MessageUtil.java Outdated
Copy link
Copy Markdown
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. One nit.

Comment thread clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java Outdated
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Sep 24, 2019

retest this please

return (short) count;
}

public static byte[] byteBufferToArray(ByteBuffer buf) {
Copy link
Copy Markdown
Member

@mumrah mumrah Sep 24, 2019

Choose a reason for hiding this comment

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

Maybe include a short javadoc here? Might be worth mentioning that this won't affect a mark on the given buffer

@cmccabe
Copy link
Copy Markdown
Contributor Author

cmccabe commented Sep 24, 2019

git failed on Jenkins:

14:20:27 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:1761)

@cmccabe
Copy link
Copy Markdown
Contributor Author

cmccabe commented Sep 24, 2019

retest this please

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.

LGTM, thanks Colin!

@cmccabe cmccabe merged commit 1d7f0b7 into apache:trunk Sep 25, 2019
ijuma added a commit to confluentinc/kafka that referenced this pull request Sep 29, 2019
Conflicts:
* .gitignore: addition of clients/src/generated-test was near
local additions for support-metrics.
* checkstyle/suppressions.xml: upstream refactoring of exclusions
for generator were near the local changes for support-metrics.
* gradle.properties: scala version bump caused a minor conflict
due to the kafka version change locally.
gradle/dependencies.gradle: bcpkix version bump was near avro
additions in the local version.

* apache-github/trunk: (49 commits)
  KAFKA-8471: Replace control requests/responses with automated protocol (apache#7353)
  MINOR: Don't generate unnecessary strings for debug logging in FetchSessionHandler (apache#7394)
  MINOR:fixed typo and removed outdated varilable name (apache#7402)
  KAFKA-8934: Create version file during build for Streams (apache#7397)
  KAFKA-8319: Make KafkaStreamsTest a non-integration test class (apache#7382)
  KAFKA-6883: Add toUpperCase support to sasl.kerberos.principal.to.local rule (KIP-309)
  KAFKA-8907; Return topic configs in CreateTopics response (KIP-525) (apache#7380)
  MINOR: Address review comments for KIP-504 authorizer changes (apache#7379)
  MINOR: add versioning to request and response headers (apache#7372)
  KAFKA-7273: Extend Connect Converter to support headers (apache#6362)
  MINOR: improve the Kafka RPC code generator (apache#7340)
  MINOR: Improve the org.apache.kafka.common.protocol code (apache#7344)
  KAFKA-8880: Docs on upgrade-guide (apache#7385)
  KAFKA-8179: do not suspend standby tasks during rebalance (apache#7321)
  KAFKA-8580: Compute RocksDB metrics (apache#7263)
  KAFKA-8880: Add overloaded function of Consumer.committed (apache#7304)
  HOTFIX: fix Kafka Streams upgrade note for broker backward compatibility (apache#7363)
  KAFKA-8848; Update system tests to use new AclAuthorizer (apache#7374)
  MINOR: remove unnecessary null check (apache#7299)
  KAFKA-6958: Overload methods for group and windowed stream to allow to name operation name using the new Named class (apache#6413)
  ...
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