Skip to content

KAFKA-4203: Align broker default for max.message.bytes with Java producer default#4154

Merged
guozhangwang merged 2 commits intoapache:trunkfrom
ijuma:kafka-4203-align-broker-max-message-bytes-with-java-producer
Jan 29, 2020
Merged

KAFKA-4203: Align broker default for max.message.bytes with Java producer default#4154
guozhangwang merged 2 commits intoapache:trunkfrom
ijuma:kafka-4203-align-broker-max-message-bytes-with-java-producer

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Oct 28, 2017

Also:

  • Improve error message
  • Add test
  • Minor code quality fixes

Verified that the test fails if the broker default for max message bytes
is lower or higher than the currently set value.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Oct 28, 2017

Review by @hachikuji

@tedyu
Copy link
Copy Markdown
Contributor

tedyu commented Oct 28, 2017

lgtm

@guozhangwang
Copy link
Copy Markdown
Contributor

LGTM

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.

LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually just one comment. Is the producer also consistent in accounting for the message set overhead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think so, but I didn't merge after Guozhang's review because I intend to write a test to ensure this is correct.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be surprising if the numbers lined up perfectly since you'd also have to account for the request overhead apparently.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't use request size despite the name

int serializedSize = AbstractRecords.estimateSizeInBytesUpperBound(apiVersions.maxUsableProduceMagic(),
                    compressionType, serializedKey, serializedValue, headers);
            ensureValidRecordSize(serializedSize);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm.. should we fix the config doc in that case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Copy Markdown
Contributor

@viktorsomogyi viktorsomogyi left a comment

Choose a reason for hiding this comment

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

Reviewed it, LGTM :)

@ijuma ijuma force-pushed the kafka-4203-align-broker-max-message-bytes-with-java-producer branch from 15cf53b to f0a7889 Compare January 29, 2020 09:09
@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 29, 2020

@guozhangwang can you please take a look at these minor updates after more than 2 years? :)

@guozhangwang
Copy link
Copy Markdown
Contributor

Haha ack :) will take a look.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Jan 29, 2020

One job passed, one failed with an unrelated known flake:

org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication

Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

LGTM!

@guozhangwang guozhangwang merged commit bd5a1c4 into apache:trunk Jan 29, 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)
  ...
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.

6 participants