Skip to content

KAFKA-10730: KafkaApis#handleProduceRequest should use auto-generated protocol #18216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

FrankYang0529
Copy link
Member

This is follow-up of KAFKA-9628 the construction of ProduceResponse is able to accept auto-generated protocol data so KafkaApis#handleProduceRequest should apply auto-generated protocol to avoid extra conversion.

Committer Checklist (excluded from commit message)

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

@ijuma
Copy link
Member

ijuma commented Dec 17, 2024

This added twice as much code as it removed - the benefit doesn't seem to justify the cost. If we want to do this, we should avoid all the extra verbosity.

@FrankYang0529
Copy link
Member Author

FrankYang0529 commented Dec 17, 2024

This added twice as much code as it removed

Old code only uses single line to represent a response, but new structure use builder pattern, so it takes more lines.

Old one:

new ProduceResponse.PartitionResponse(error, offset, RecordBatch.NO_TIMESTAMP, logStartOffset);

New one:

new ProduceResponseData.PartitionProduceResponse()
    .setIndex(tp.partition())
    .setErrorCode(error.code())
    .setBaseOffset(offset)
    .setLogStartOffset(logStartOffset)
    .setLogAppendTimeMs(RecordBatch.NO_TIMESTAMP)

@ijuma
Copy link
Member

ijuma commented Dec 22, 2024

Right, so why do we want to do this? What's the benefit?

@FrankYang0529
Copy link
Member Author

Hi @ijuma, thanks for the review. The good part is that we eliminate data conversion time. We don't need to convert data from PartitionResponse to ProduceResponseData.PartitionProduceResponse.

Before (trunk):

Result "org.apache.kafka.jmh.producer.ProducerResponseBenchmark.constructorProduceResponse":
  209.285 ±(99.9%) 1.822 ns/op [Average]
  (min, avg, max) = (204.419, 209.285, 211.999), stdev = 1.705
  CI (99.9%): [207.463, 211.108] (assumes normal distribution)

After (this branch):

Result "org.apache.kafka.jmh.producer.ProducerResponseBenchmark.constructorProduceResponse":
  139.632 ±(99.9%) 36.446 ns/op [Average]
  (min, avg, max) = (67.455, 139.632, 173.913), stdev = 34.091
  CI (99.9%): [103.187, 176.078] (assumes normal distribution)

@FrankYang0529
Copy link
Member Author

Hi @chia7712 @ijuma, may you help me review this when you have time? Thanks.

@FrankYang0529 FrankYang0529 force-pushed the KAFKA-10730 branch 7 times, most recently from 17e5fdb to 3248a96 Compare January 18, 2025 04:17
@FrankYang0529 FrankYang0529 force-pushed the KAFKA-10730 branch 5 times, most recently from 37d10f8 to ae77bd1 Compare January 30, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants