[GitHub] [kafka] cadonna commented on a diff in pull request #13127: KAFKA-14586: Moving StreamResetter to tools
cadonna commented on code in PR #13127: URL: https://github.com/apache/kafka/pull/13127#discussion_r1149019632 ## streams/src/test/java/org/apache/kafka/streams/tools/StreamsResetterTest.java: ## @@ -16,7 +16,7 @@ */ package org.apache.kafka.streams.tools; -import kafka.tools.StreamsResetter; +import org.apache.kafka.tools.StreamsResetter; Review Comment: I would not wait for the answer. We can do that in a follow-up PR. Could you open a ticket for the move? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #13127: KAFKA-14586: Moving StreamResetter to tools
cadonna commented on code in PR #13127: URL: https://github.com/apache/kafka/pull/13127#discussion_r1105688687 ## streams/src/test/java/org/apache/kafka/streams/tools/StreamsResetterTest.java: ## @@ -16,7 +16,7 @@ */ package org.apache.kafka.streams.tools; -import kafka.tools.StreamsResetter; +import org.apache.kafka.tools.StreamsResetter; Review Comment: I would also move it to the tools module. @mjsax @guozhangwang Was there a specific reason not to have the test in the same module as the `StreamsResetter`? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #13127: KAFKA-14586: Moving StreamResetter to tools
cadonna commented on code in PR #13127: URL: https://github.com/apache/kafka/pull/13127#discussion_r1105688687 ## streams/src/test/java/org/apache/kafka/streams/tools/StreamsResetterTest.java: ## @@ -16,7 +16,7 @@ */ package org.apache.kafka.streams.tools; -import kafka.tools.StreamsResetter; +import org.apache.kafka.tools.StreamsResetter; Review Comment: I would also move it to the tools module. @mjsax Was there a specific reason not to have the test in the same module as the `StreamsResetter`? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #13127: Kafka 14586: Moving StreamResetter to tools
cadonna commented on code in PR #13127:
URL: https://github.com/apache/kafka/pull/13127#discussion_r1081015656
##
build.gradle:
##
@@ -1757,6 +1757,7 @@ project(':tools') {
archivesBaseName = "kafka-tools"
dependencies {
+implementation project(':core')
Review Comment:
Ah, great! Thanks for sharing! Good to know that people is aware!
> Also, not sure if this comment from Ismael has any relevance here:
https://github.com/apache/kafka/pull/13095#issuecomment-1376021193?
I think there they talk about the dependencies of tools' test module not of
the tools per se.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] cadonna commented on a diff in pull request #13127: Kafka 14586: Moving StreamResetter to tools
cadonna commented on code in PR #13127:
URL: https://github.com/apache/kafka/pull/13127#discussion_r1081000404
##
build.gradle:
##
@@ -1757,6 +1757,7 @@ project(':tools') {
archivesBaseName = "kafka-tools"
dependencies {
+implementation project(':core')
Review Comment:
Ticket https://issues.apache.org/jira/browse/KAFKA-14525 (the parent of
https://issues.apache.org/jira/browse/KAFKA-14586) says
> tools that don't require access to `core` classes and communicate via the
kafka protocol (typically by using the client classes) should be moved to the
`tools` module.
This addition contradicts that requirement.
As far as I see, `kafka.utils.CommandLineUtils` is the only dependency to
`core`. Is that true? Would it be possible to move
`kafka.utils.CommandLineUtils` to a different module? Or should we even get
completely rid of that dependency by rewriting it in java and put it in a
different module.
https://issues.apache.org/jira/browse/KAFKA-14576 should have a similar
issue since also the console consumer should be moved to tools and it has a
dependency to `kafka.utils.CommandLineUtils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
