Skip to content

KAFKA-9474: Adds 'float64' to the RPC protocol types.#8012

Merged
ijuma merged 4 commits intoapache:trunkfrom
bdbyrne:KAFKA-9474
Jan 30, 2020
Merged

KAFKA-9474: Adds 'float64' to the RPC protocol types.#8012
ijuma merged 4 commits intoapache:trunkfrom
bdbyrne:KAFKA-9474

Conversation

@bdbyrne
Copy link
Copy Markdown
Contributor

@bdbyrne bdbyrne commented Jan 28, 2020

Committer Checklist (excluded from commit message)

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

Copy link
Copy Markdown
Contributor

@hachikuji hachikuji 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 at a first pass. Is it worth adding the new field type to clients/src/test/resources/common/message/SimpleExampleMessage.json?

Comment thread clients/src/main/resources/common/message/README.md Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/utils/ByteUtils.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/utils/ByteUtils.java Outdated
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 28, 2020

ok to test

@bdbyrne bdbyrne changed the title KAFKA-9474: Adds 'double' to the RPC protocol types. KAFKA-9474: Adds 'float64' to the RPC protocol types. Jan 29, 2020
@bdbyrne
Copy link
Copy Markdown
Contributor Author

bdbyrne commented Jan 29, 2020

Looks good at a first pass. Is it worth adding the new field type to clients/src/test/resources/common/message/SimpleExampleMessage.json?

Added an entry.

Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

This is looking pretty good to me. A few minor inline comments below.

Comment thread generator/src/main/java/org/apache/kafka/message/MessageDataGenerator.java Outdated
@Override
public String documentation() {
return "Represents a double-precision 64-bit format IEEE 754 value. " +
"The values are encoded using eight bytes in network byte order (big-endian).";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For my benefit, what was the reasoning for choosing big endian?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For consistency - all of the integers types are big-endian as well.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 29, 2020

add to whitelist

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 29, 2020

ok to test

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 29, 2020

retest this please

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 29, 2020

ok to test

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 29, 2020

retest this please

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jan 30, 2020

One job passed, one failed with two unrelated flakes:

kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdEmptyGroup
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsExportImportPlan

Copy link
Copy Markdown
Member

@ijuma ijuma 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. I will go ahead and merge this. If @cmccabe has additional comments, we can address them in the PR where we actually use this new type.

@ijuma ijuma merged commit 57cef76 into apache:trunk Jan 30, 2020
ijuma added a commit to confluentinc/kafka that referenced this pull request Feb 2, 2020
Conflicts and/or compiler errors due to the fact that we
temporarily reverted the commit that removes
Scala 2.11 support:

* SslAdminIntegrationTest: keep using JAdminClient,
take upstream changes otherwise.
* ReassignPartitionsClusterTest: keep using
JAdminClient, take upstream changes otherwise.
* KafkaApis: use `asScala.foreach` instead of
`forEach`.

# By Ismael Juma (3) and others
# Via GitHub
* apache-github/trunk: (22 commits)
  KAFKA-9437; Make the Kafka Protocol Friendlier with L7 Proxies [KIP-559] (apache#7994)
  KAFKA-9375: Add names to all Connect threads (apache#7901)
  MINOR: Introduce 2.5-IV0 IBP (apache#8010)
  KAFKA-8503; Add default api timeout to AdminClient (KIP-533) (apache#8011)
  Add retries to release.py script (apache#8021)
  KAFKA-8162: IBM JDK Class not found error when handling SASL (apache#6524)
  MINOR: Add explicit result type in public defs/vals (apache#7993)
  KAFKA-9408: Use StandardCharsets.UTF-8 instead of "UTF-8" (apache#7940)
  KAFKA-9474: Adds 'float64' to the RPC protocol types (apache#8012)
  KAFKA-9360: Allow disabling MM2 heartbeat and checkpoint emissions (apache#7887)
  KAFKA-7658: Add KStream#toTable to the Streams DSL (apache#7985)
  KAFKA-9445: Allow adding changes to allow serving from a specific partition (apache#7984)
  KAFKA-9422: Track the set of topics a connector is using (KIP-558) (apache#8017)
  KAFKA-9040; Add --all option to config command (apache#7607)
  KAFKA-4203: Align broker default for max.message.bytes with Java producer default (apache#4154)
  KAFKA-9426: Use switch instead of chained if/else in OffsetsForLeaderEpochClient (apache#7959)
  KAFKA-9405: Use Map.computeIfAbsent where applicable (apache#7937)
  KAFKA-9026: Use automatic RPC generation in DescribeAcls (apache#7560)
  MINOR: Remove unused fields in StreamsMetricsImpl (apache#7992)
  KAFKA-9460: Enable only TLSv1.2 by default and disable other TLS protocol versions (KIP-553) (apache#7998)
  ...
gitlw pushed a commit to linkedin/kafka that referenced this pull request Sep 9, 2021
…ocol types (apache#8012)

Reviewers: Jason Gustafson <jason@confluent.io>, Ismael Juma <ismael@juma.me.uk>
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.

3 participants