You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/16 14:04:05 UTC
[GitHub] [kafka] MarcoLotz opened a new pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
MarcoLotz opened a new pull request #10131:
URL: https://github.com/apache/kafka/pull/10131
Moved "streams-examples" to its own module outside kafka-streams module.
Because of org.apache.kafka.streams.processor.internals.StateDirectory in kafka-streams module, I had to add the jackson binder dependency. Before the change, It was probably being retrieved as a transitive dependency through "connect-json" dependency.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r587501265
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
@ableegoldman No. I removed the `connect-json` dependency from `streams` and left it in `streams:examples` and there is no longer a `streams` dependency:
> compileClasspath - Compile classpath for source set 'main'.
> +--- project :clients
> +--- org.slf4j:slf4j-api:1.7.30
> +--- org.rocksdb:rocksdbjni:5.18.4
> +--- com.fasterxml.jackson.core:jackson-annotations:2.10.5
> \--- com.fasterxml.jackson.core:jackson-databind:2.10.5.1
> +--- com.fasterxml.jackson.core:jackson-annotations:2.10.5
> \--- com.fasterxml.jackson.core:jackson-core:2.10.5
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r591997331
##########
File path: build.gradle
##########
@@ -1416,14 +1416,6 @@ project(':streams') {
dependencies {
api project(':clients')
- // use `api` dependency for `connect-json` for compatibility (e.g. users who use `JsonSerializer`/`JsonDeserializer`
- // at compile-time without an explicit dependency on `connect-json`)
- // this dependency should be removed after we unify data API
- api(project(':connect:json')) {
- // this transitive dependency is not used in Streams, and it breaks SBT builds
- exclude module: 'javax.ws.rs-api'
- }
-
implementation libs.slf4jApi
implementation libs.rocksDBJni
implementation libs.jacksonAnnotations
Review comment:
SGTM.
Back in the days, KafkaStreams did not use anything JSON related, but in a later version we actually added this dependency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r587511329
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
Sounds good. I will send a PR later this week fixing it to be as discussed here then. Probably even tonight (CET).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#issuecomment-808941959
@MarcoLotz Overall LGTM -- but as mentioned by @ijuma (cf https://github.com/apache/kafka/pull/10131#discussion_r586670861) we should add a comment to the upgrade note (ie, in `docs/upgrade.html` as well as `docs/streams/upgrade-guide.html`) -- there is already a corresponding section for the `3.0.0` release in both (just add one bullet point.)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r588864401
##########
File path: build.gradle
##########
@@ -1416,14 +1416,6 @@ project(':streams') {
dependencies {
api project(':clients')
- // use `api` dependency for `connect-json` for compatibility (e.g. users who use `JsonSerializer`/`JsonDeserializer`
- // at compile-time without an explicit dependency on `connect-json`)
- // this dependency should be removed after we unify data API
- api(project(':connect:json')) {
- // this transitive dependency is not used in Streams, and it breaks SBT builds
- exclude module: 'javax.ws.rs-api'
- }
-
implementation libs.slf4jApi
implementation libs.rocksDBJni
implementation libs.jacksonAnnotations
Review comment:
"implementation libs.jacksonDatabind" is already being added to compileClassPath in streams module.
Thus it seems there indeed there's no need to add compile dependency of libs.jacksonDatabind to streams module. Please let me know if this is not the case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r607979819
##########
File path: docs/upgrade.html
##########
@@ -37,6 +37,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
<li>The deprecated <code>kafka-preferred-replica-election</code> command line tool was removed. Please use <code>kafka-leader-election</code> instead.</li>
<li>The deprecated <code>ConfigEntry</code> constructor was removed (<a href="https://issues.apache.org/jira/browse/KAFKA-12577">KAFKA-12577</a>).
Please use the remaining public constructor instead.</li>
+ <li>Kafka Streams no longer has a compile time dependency on "connect:json" module (<a href="https://issues.apache.org/jira/browse/KAFKA-5146">KAFKA-5146</a>).
Review comment:
Not: For the top level docs, it might be ok to remove the link to the ticket?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r608024597
##########
File path: docs/upgrade.html
##########
@@ -37,6 +37,8 @@ <h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3
<li>The deprecated <code>kafka-preferred-replica-election</code> command line tool was removed. Please use <code>kafka-leader-election</code> instead.</li>
<li>The deprecated <code>ConfigEntry</code> constructor was removed (<a href="https://issues.apache.org/jira/browse/KAFKA-12577">KAFKA-12577</a>).
Please use the remaining public constructor instead.</li>
+ <li>Kafka Streams no longer has a compile time dependency on "connect:json" module (<a href="https://issues.apache.org/jira/browse/KAFKA-5146">KAFKA-5146</a>).
Review comment:
In docs/upgrade.html they all reference a Kafka jira ticket with the hyperlink. In docs/streams/upgrade-guide.html they all reference a KIP, but there were previous upgrades referencing a jira ticket with the hyperlink. Seems consistent to keep hyperlink in both files.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#issuecomment-789939535
We have a larger PR in-progress that changes the gradle file significantly, and I expect conflicts (#10203). Wondering if we can just merge this PR or if it would be better to merge #10203 first and rebase this PR? \cc @ijuma
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r588864401
##########
File path: build.gradle
##########
@@ -1416,14 +1416,6 @@ project(':streams') {
dependencies {
api project(':clients')
- // use `api` dependency for `connect-json` for compatibility (e.g. users who use `JsonSerializer`/`JsonDeserializer`
- // at compile-time without an explicit dependency on `connect-json`)
- // this dependency should be removed after we unify data API
- api(project(':connect:json')) {
- // this transitive dependency is not used in Streams, and it breaks SBT builds
- exclude module: 'javax.ws.rs-api'
- }
-
implementation libs.slf4jApi
implementation libs.rocksDBJni
implementation libs.jacksonAnnotations
Review comment:
"implementation libs.jacksonDatabind" is already being added to compileClassPath in streams module.
Thus it seems that indeed there's no need to add compile dependency of libs.jacksonDatabind to streams module. Please let me know if this is not the case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r587502355
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
I think that's all you need to do.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586868205
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
That seems independent of whether it's a submodule or not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586715706
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
Is there an advantage to having it as a top level module instead?
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
Is there an advantage in having it as a top level module instead?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586651685
##########
File path: build.gradle
##########
@@ -1528,8 +1523,7 @@ project(':streams') {
':streams:upgrade-system-tests-24:test',
':streams:upgrade-system-tests-25:test',
':streams:upgrade-system-tests-26:test',
- ':streams:upgrade-system-tests-27:test',
- ':streams:examples:test'
Review comment:
I think we should include the new `streams-examples` module in `testAll` goal.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r608024857
##########
File path: build.gradle
##########
@@ -1423,18 +1423,8 @@ project(':streams') {
dependencies {
api project(':clients')
- // use `api` dependency for `connect-json` for compatibility (e.g. users who use `JsonSerializer`/`JsonDeserializer`
- // at compile-time without an explicit dependency on `connect-json`)
- // this dependency should be removed after we unify data API
- api(project(':connect:json')) {
- // this transitive dependency is not used in Streams, and it breaks SBT builds
- exclude module: 'javax.ws.rs-api'
- }
-
- // `org.rocksdb.Options` is part of Kafka Streams public api via `RocksDBConfigSetter`
- api libs.rocksDBJni
-
implementation libs.slf4jApi
+ implementation libs.rocksDBJni
Review comment:
Indeed unintentional. fixed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586652341
##########
File path: streams-examples/.gitignore
##########
@@ -0,0 +1 @@
+/bin/
Review comment:
Just curious. Why do we need this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#issuecomment-814281424
> Sorry the delay -- and thanks for updating the docs! Overall LGTM.
>
> Seems there is some conflict -- can you rebase so we can merge this PR?
Sure thing @mjsax ! Fixed merge conflicts and extra comment from @ijuma
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#issuecomment-808963914
Thank you for pointing the directions @mjsax. Updated those files. Since it's not API related, I've used another paragraph on streams/upgrade-guide.html
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586863239
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
As describe on the Jira, the goal is to get rid of the dependency of `stream` model on `connect` module. It's ok for `streams-examples` to depend on it, but for `streams` is cleaner to not have it, and it's only there to pull in stuff for the examples.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586711923
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
We move the sub-module `stream/examples` into it's own module `streams-examples`. I would be weird to only name it `examples` as there is not parent `streams` reference any longer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586671249
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
Why did we change the name?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#issuecomment-805936369
@mjsax @ijuma I have just updated the PR to fix merge-conflicts. Since the build file changes so often, would it be possible for you to have a quick look into it and verify if everything is as expected?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586863239
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
As describe on the Jira, the goal is to get rid of the dependency of `stream` model on `connect` module. It's ok for `streams-examples` to depend on `connect`, but for `streams` is cleaner to not have it, and the dependency is only there to pull in stuff for the examples.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586670861
##########
File path: build.gradle
##########
@@ -1403,12 +1403,7 @@ project(':streams') {
dependencies {
compile project(':clients')
- // this dependency should be removed after we unify data API
- compile(project(':connect:json')) {
- // this transitive dependency is not used in Streams, and it breaks SBT builds
- exclude module: 'javax.ws.rs-api'
- }
Review comment:
This will break users who were relying on the transitive dependency. We may want to add a note to the release notes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r587501265
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
@ableegoldman No. I removed the `connect-json` dependency from `streams` and left it in `streams:examples` and the dependency is gone from the `streams` module:
> compileClasspath - Compile classpath for source set 'main'.
> +--- project :clients
> +--- org.slf4j:slf4j-api:1.7.30
> +--- org.rocksdb:rocksdbjni:5.18.4
> +--- com.fasterxml.jackson.core:jackson-annotations:2.10.5
> \--- com.fasterxml.jackson.core:jackson-databind:2.10.5.1
> +--- com.fasterxml.jackson.core:jackson-annotations:2.10.5
> \--- com.fasterxml.jackson.core:jackson-core:2.10.5
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r592126995
##########
File path: build.gradle
##########
@@ -1635,8 +1627,14 @@ project(':streams:examples') {
archivesBaseName = "kafka-streams-examples"
dependencies {
+ // this dependency should be removed after we unify data API
+ compile(project(':connect:json')) {
Review comment:
@ijuma well noted! I have updated the PR. Since there are many "compile" references in build.gradle, I will have a look later on JIRA if there is already a ticket to update all this to api or implementation and raise if not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586918859
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
Huh, maybe we're not even using `connect` in the examples to begin with (or at least, not anymore)? That actually doesn't sound too surprising, I've never seen Connect being used in any streams stuff I've looked at
> That seems independent of whether it's a submodule or not
@ijuma WDYM? If the examples are a submodule of `streams`, and the examples depend on `connect`, then doesn't that mean the `streams` module will pull in `connect`? Even if we may turn out not to depend on `connect` to begin with...)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r592000778
##########
File path: build.gradle
##########
@@ -1635,8 +1627,14 @@ project(':streams:examples') {
archivesBaseName = "kafka-streams-examples"
dependencies {
+ // this dependency should be removed after we unify data API
+ compile(project(':connect:json')) {
Review comment:
`compile` is deprecated, you have to choose between `api` and `implementation`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] MarcoLotz commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
MarcoLotz commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r592169117
##########
File path: build.gradle
##########
@@ -1635,8 +1627,14 @@ project(':streams:examples') {
archivesBaseName = "kafka-streams-examples"
dependencies {
+ // this dependency should be removed after we unify data API
+ compile(project(':connect:json')) {
Review comment:
having a better look, seems that no ticket is required. They are compileOnly or testCompileOnly which are not deprecated. All good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r586900476
##########
File path: build.gradle
##########
@@ -974,8 +974,8 @@ project(':core') {
from(project(':streams:streams-scala').configurations.runtime) { into("libs/") }
from(project(':streams:test-utils').jar) { into("libs/") }
from(project(':streams:test-utils').configurations.runtime) { into("libs/") }
- from(project(':streams:examples').jar) { into("libs/") }
- from(project(':streams:examples').configurations.runtime) { into("libs/") }
+ from(project(':streams-examples').jar) { into("libs/") }
+ from(project(':streams-examples').configurations.runtime) { into("libs/") }
Review comment:
Interesting. \cc @ableegoldman
I actually just tried to only remove the `connect` dependency from `streams` module (and add the Jackson dependency we need fo `StateDirectory.java`) and it seems to work.
So maybe we can just keep sub-module `:streams:examples` as-is?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#discussion_r602917794
##########
File path: build.gradle
##########
@@ -1423,18 +1423,8 @@ project(':streams') {
dependencies {
api project(':clients')
- // use `api` dependency for `connect-json` for compatibility (e.g. users who use `JsonSerializer`/`JsonDeserializer`
- // at compile-time without an explicit dependency on `connect-json`)
- // this dependency should be removed after we unify data API
- api(project(':connect:json')) {
- // this transitive dependency is not used in Streams, and it breaks SBT builds
- exclude module: 'javax.ws.rs-api'
- }
-
- // `org.rocksdb.Options` is part of Kafka Streams public api via `RocksDBConfigSetter`
- api libs.rocksDBJni
-
implementation libs.slf4jApi
+ implementation libs.rocksDBJni
Review comment:
This seems wrong, is it unintentional?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #10131:
URL: https://github.com/apache/kafka/pull/10131#issuecomment-814523321
Thanks for the PR @MarcoLotz
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax merged pull request #10131: KAFKA-5146 / move kafka-streams example to a new module
Posted by GitBox <gi...@apache.org>.
mjsax merged pull request #10131:
URL: https://github.com/apache/kafka/pull/10131
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org