KAFKA-5146 / move kafka-streams example to a new module #10131

MarcoLotz · 2021-02-16T14:03:52Z

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)

ableegoldman

LGTM, thanks for the PR!

mjsax · 2021-03-03T18:00:47Z

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

mjsax · 2021-03-03T18:02:18Z

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'


I think we should include the new streams-examples module in testAll goal.

mjsax · 2021-03-03T18:03:18Z

streams-examples/.gitignore

@@ -0,0 +1 @@
+/bin/


Just curious. Why do we need this?

ijuma · 2021-03-03T18:20:23Z

build.gradle

-    compile(project(':connect:json')) {
-      // this transitive dependency is not used in Streams, and it breaks SBT builds
-      exclude module: 'javax.ws.rs-api'
-    }


This will break users who were relying on the transitive dependency. We may want to add a note to the release notes.

ijuma · 2021-03-03T18:20:58Z

build.gradle

-    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/") }


Why did we change the name?

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.

Is there an advantage in having it as a top level module instead?

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.

That seems independent of whether it's a submodule or not.

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?

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...)

@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

I think that's all you need to do.

Sounds good. I will send a PR later this week fixing it to be as discussed here then. Probably even tonight (CET).

MarcoLotz · 2021-03-06T10:25:00Z

build.gradle

-      // 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


"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.

SGTM.

Back in the days, KafkaStreams did not use anything JSON related, but in a later version we actually added this dependency.

ijuma · 2021-03-11T01:29:16Z

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')) {


compile is deprecated, you have to choose between api and implementation.

@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.

having a better look, seems that no ticket is required. They are compileOnly or testCompileOnly which are not deprecated. All good.

MarcoLotz · 2021-03-24T15:44:59Z

@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?

mjsax · 2021-03-28T18:56:04Z

@MarcoLotz Overall LGTM -- but as mentioned by @ijuma (cf #10131 (comment)) 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.)

MarcoLotz · 2021-03-28T21:42:19Z

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

mjsax

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?

docs/upgrade.html

build.gradle

… sub-module

MarcoLotz · 2021-04-06T17:03:07Z

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

mjsax · 2021-04-07T00:48:04Z

Thanks for the PR @MarcoLotz

* apache-github/trunk: KAFKA-10769 Remove JoinGroupRequest#containsValidPattern as it is dup… (apache#9851) KAFKA-12384: stabilize ListOffsetsRequestTest#testResponseIncludesLeaderEpoch (apache#10389) KAFKA-5146: remove Connect dependency from Streams module (apache#10131)

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch from 0681b34 to e568571 Compare February 16, 2021 14:06

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch from e568571 to cae4e15 Compare March 1, 2021 21:21

ableegoldman approved these changes Mar 3, 2021

View reviewed changes

mjsax added the streams label Mar 3, 2021

mjsax reviewed Mar 3, 2021

View reviewed changes

streams-examples/.gitignore Outdated

@@ -0,0 +1 @@

/bin/

Copy link

Member

mjsax Mar 3, 2021

Choose a reason for hiding this comment

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

Just curious. Why do we need this?

ijuma reviewed Mar 3, 2021

View reviewed changes

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch from cae4e15 to 75879da Compare March 6, 2021 10:04

MarcoLotz commented Mar 6, 2021

View reviewed changes

ijuma reviewed Mar 11, 2021

View reviewed changes

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch from 75879da to c2cb194 Compare March 11, 2021 07:38

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch from c2cb194 to 8c439e7 Compare March 24, 2021 15:40

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch from 0e50a83 to cb3be19 Compare March 28, 2021 21:39

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch 2 times, most recently from 215b74a to bae4795 Compare April 6, 2021 14:41

mjsax reviewed Apr 6, 2021

View reviewed changes

docs/upgrade.html Outdated Show resolved Hide resolved

ijuma reviewed Apr 6, 2021

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

KAFKA-5146 / move connect dependency from streams to streams-examples…

dd605f7

… sub-module

MarcoLotz force-pushed the bugfix/KAFKA-5146 branch from bae4795 to dd605f7 Compare April 6, 2021 16:54

mjsax merged commit 33d0445 into apache:trunk Apr 7, 2021

KAFKA-5146 / move kafka-streams example to a new module #10131

KAFKA-5146 / move kafka-streams example to a new module #10131

Uh oh!

Conversation

MarcoLotz commented Feb 16, 2021

Committer Checklist (excluded from commit message)

Uh oh!

ableegoldman left a comment

Choose a reason for hiding this comment

Uh oh!

mjsax commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoLotz Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoLotz commented Mar 24, 2021

Uh oh!

mjsax commented Mar 28, 2021

Uh oh!

MarcoLotz commented Mar 28, 2021

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarcoLotz commented Apr 6, 2021

Uh oh!

mjsax commented Apr 7, 2021

Uh oh!

Uh oh!

ijuma Mar 3, 2021 •

edited

Loading

mjsax Mar 3, 2021 •

edited

Loading

ijuma Mar 4, 2021 •

edited

Loading

MarcoLotz Mar 6, 2021 •

edited

Loading