Skip to content

KAFKA-10244 An new java interface to replace 'kafka.common.MessageReader'#13393

Merged
chia7712 merged 12 commits intoapache:trunkfrom
chia7712:KAFKA-10244
Mar 25, 2023
Merged

KAFKA-10244 An new java interface to replace 'kafka.common.MessageReader'#13393
chia7712 merged 12 commits intoapache:trunkfrom
chia7712:KAFKA-10244

Conversation

@chia7712
Copy link
Copy Markdown
Member

related to https://issues.apache.org/jira/browse/KAFKA-10244

kafka.common.MessageReader is a input argument of kafka-console-producer and we expect users can have their custom reader to produce custom records. Hence, MessageReader is a public interface and we should offer a java version to replace current scala code. Also, the new MessageReader should be placed at clients module. (kafka.common.MessageReader is in core module)

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
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left a few comments.

Comment thread clients/src/main/java/org/apache/kafka/tools/RecordReader.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/tools/RecordReader.java
Comment thread core/src/main/scala/kafka/tools/ConsoleProducer.scala Outdated
Comment thread core/src/main/scala/kafka/tools/ConsoleProducer.scala Outdated
Comment thread core/src/test/scala/unit/kafka/tools/ConsoleProducerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/tools/ConsoleProducerTest.scala Outdated
Comment thread core/src/main/scala/kafka/tools/ConsoleProducer.scala Outdated
@chia7712
Copy link
Copy Markdown
Member Author

@mimaison thanks for reviews. I have addressed all comments. please take a look, thanks!

Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. LGTM

@chia7712
Copy link
Copy Markdown
Member Author


[2023-03-23T20:27:39.266Z] Gradle Test Run :core:integrationTest > Gradle Test Executor 182 > PlaintextAdminIntegrationTest > testReplicaCanFetchFromLogStartOffsetAfterDeleteRecords(String) > kafka.api.PlaintextAdminIntegrationTest.testReplicaCanFetchFromLogStartOffsetAfterDeleteRecords(String)[2] FAILED
[2023-03-23T20:27:39.266Z]     java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.TimeoutException: The request timed out.
[2023-03-23T20:27:39.266Z]         at java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:357)
[2023-03-23T20:27:39.266Z]         at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1908)
[2023-03-23T20:27:39.266Z]         at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:165)
[2023-03-23T20:27:39.266Z]         at kafka.api.PlaintextAdminIntegrationTest.testReplicaCanFetchFromLogStartOffsetAfterDeleteRecords(PlaintextAdminIntegrationTest.scala:859)
[2023-03-23T20:27:39.266Z] 
[2023-03-23T20:27:39.266Z]         Caused by:
[2023-03-23T20:27:39.267Z]         org.apache.kafka.common.errors.TimeoutException: The request timed out.

unrelated error. will merge it later

@chia7712 chia7712 merged commit 18fba41 into apache:trunk Mar 25, 2023
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Mar 25, 2023

Since this is a public api, we need to enable javadoc generation for it. Have we done that?

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Mar 25, 2023

One additional issue is that we are introducing a new "split packages" situation. We already have some of those, but we should probably not add new ones for public apis due to https://www.logicbig.com/tutorials/core-java-tutorial/modules/split-packages.html

@chia7712
Copy link
Copy Markdown
Member Author

we need to enable javadoc generation for it. Have we done that?

I will address it later.

should probably not add new ones for public apis due to https://www.logicbig.com/tutorials/core-java-tutorial/modules/split-packages.html

thanks for nice reminder. the alternative is org.apache.kafka.common ( same as MessageFormatter). The side effect is that we have to add allow rule for importing ProducerRecord. WDYT?

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