MINOR: move RecordReader from org.apache.kafka.tools (client module) to org.apache.kafka.tools.api (tools-api module)#13454
Conversation
|
@ijuma ping for review :) I will update KIP if this change is acceptable |
| * limitations under the License. | ||
| */ | ||
| package org.apache.kafka.tools; | ||
| package org.apache.kafka.common; |
There was a problem hiding this comment.
Another possibility is to move this to the tools module under the package org.apache.kafka.tools.api, which seems a bit cleaner?
There was a problem hiding this comment.
This would follow a similar pattern as the storage api classes, which seems reasonable to me.
There was a problem hiding this comment.
There was a problem hiding this comment.
tools doesn't depend on core, only tool tests depend on core. Right?
There was a problem hiding this comment.
tools doesn't depend on core, only tool tests depend on core. Right?
ok, will use new package instead.
There was a problem hiding this comment.
It's fine to use a separate module as you have.
There was a problem hiding this comment.
It's fine to use a separate module as you have.
@ijuma thanks for this response. I prefer to use separate module. It won't bring complex dependency cycle ( core main/test -> tool main, tool test -> core main). Also, we can control the dependencies exposed to users without impacting the tools.
|
I have test
|
| implementation project(':group-coordinator') | ||
| implementation project(':metadata') | ||
| implementation project(':storage:api') | ||
| implementation project(':tools:tools-api') |
There was a problem hiding this comment.
Gradle can't distinguish the subproject name (gradle/gradle#847), so I add the prefix tools to avoid conflict to storage:api
There was a problem hiding this comment.
Why does core depend on tools-api?
| } | ||
| } | ||
|
|
||
| project(':tools:tools-api') { |
There was a problem hiding this comment.
Should we change the javadoc part to include the tools.api package?
There was a problem hiding this comment.
I have added the tools.api package to javadoc (see https://github.com/apache/kafka/pull/13454/files#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R1823)
There was a problem hiding this comment.
The doc-related test is shown in previous comment (#13454 (comment))
| implementation project(':group-coordinator') | ||
| implementation project(':metadata') | ||
| implementation project(':storage:api') | ||
| implementation project(':tools:tools-api') |
There was a problem hiding this comment.
Why does core depend on tools-api?
ConsoleProducer is still in core module. #13214 can remove the dependency after it moves |
| public class RecordReaderTest { | ||
|
|
||
| @Test | ||
| void testDefaultCloseAndConfigure() { |
There was a problem hiding this comment.
What do we test in this unit test? There is no assertion.
There was a problem hiding this comment.
make sure the default implementation of those methods don't throw exception.
I will add assertDoesNotThrow to have it more readable.
chia7712
changed the title
| from(project(':streams:test-utils').configurations.runtimeClasspath) { into("libs/") } | ||
| from(project(':streams:examples').jar) { into("libs/") } | ||
| from(project(':streams:examples').configurations.runtimeClasspath) { into("libs/") } | ||
| from(project(':tools:tools-api').jar) { into("libs/") } |
There was a problem hiding this comment.
This causes a resolution issue. I have opened https://issues.apache.org/jira/browse/KAFKA-19750 to fix it
3 participants
from #13393 (comment)
org.apache.kafka.toolsis existent in both tools and client module, and it causes split packages (https://www.logicbig.com/tutorials/core-java-tutorial/modules/split-packages.html)test
aggregatedJavadocand the output is shown below.Committer Checklist (excluded from commit message)