GH-550: master to 2.2; fix tangles #641

garyrussell · 2018-04-03T20:27:02Z

Fixes #550

package tangle ....listener and ....listener.config - remove config package
class tangles between ContainerProperties and the listener containers
- AckMode moved to properties
- Error handler setters moved from properties to containers
class tangle between listener adapter and handler adapter due to ResultHolder

garyrussell · 2018-04-03T21:11:13Z

Do not squash commits.

garyrussell · 2018-04-04T17:26:36Z

I have created the 2.1.x branch.

Fixes spring-projects#550 - package tangle `....listener` and `....listener.config` - remove config package - class tangles between `ContainerProperties` and the listener containers - AckMode moved to properties - Error handler setters moved from properties to containers

Resolves spring-projects#640

artembilan

Also, please, add @DisabledOnOs(WINDOWS) for the StreamsBuilderFactoryBeanTests.

I'm not sure what's wrong with resources, but there is a collision during removal directory.

artembilan · 2018-04-09T20:44:23Z

spring-kafka-test/src/main/java/org/springframework/kafka/test/rule/KafkaEmbedded.java

+//		clientVersion = AppInfoParser.getVersion();
+//		if (clientVersion.startsWith("1.1.")) {
+//			try {
+//				testUtilsCreateBrokerConfigMethod = TestUtils.class.getDeclaredMethod("createBrokerConfig",


I think we don't consider to support older version on this branch. Do we ?
I mean no need to comment out, but just remove such a code.
Although I still see how we have if...else in the createBrokerProperties().
Shound't that be fixed respectively ?

Thanks

I decided to just comment it out to make it easier to reinstate if we have a similar problem with 1.2.0. I can remove it entirely if you insist.

Fair enough. Let it live as is!

artembilan · 2018-04-09T20:48:48Z

build.gradle

@@ -78,7 +78,8 @@ subprojects { subproject ->
 		junitJupiterVersion = '5.1.0'
 		junitPlatformVersion = '1.1.0'
 		junitVintageVersion = '5.1.0'
-		kafkaVersion = '1.0.1'
+		kafkaVersion = '1.1.0'
+		log4jVersion = '2.10.0'


Why do we need to support log4J2 dependency if it comes transitively from Kafka now?

Also this is not visible in the change set, but we don't need to have exclude group: 'org.slf4j', module: 'slf4j-log4j12' anymore. At least that is how I understand it from your code change.

BTW, do you cover here a fix for the #278 ?

Thanks

It doesn't come from kafka - they use slf4j now. https://kafka.apache.org/documentation/#upgrade_110_notable

Yes; I switched to log4j2.

Ah! Cool!

Then we don't need to support slf dependency. We should use log4j-slf4j-impl instead: https://logging.apache.org/log4j/2.x/log4j-slf4j-impl/index.html

Also pay attention how there is already Log4j-2.11.0

However I still think we don't need those slf excludes anymore.

artembilan · 2018-04-10T00:20:24Z

build.gradle

@@ -78,7 +78,8 @@ subprojects { subproject ->
 		junitJupiterVersion = '5.1.0'
 		junitPlatformVersion = '1.1.0'
 		junitVintageVersion = '5.1.0'
-		kafkaVersion = '1.0.1'
+		kafkaVersion = '1.1.0'
+		log4jVersion = '2.10.0'


Ah! Cool!

Then we don't need to support slf dependency. We should use log4j-slf4j-impl instead: https://logging.apache.org/log4j/2.x/log4j-slf4j-impl/index.html

Also pay attention how there is already Log4j-2.11.0

However I still think we don't need those slf excludes anymore.

artembilan · 2018-04-10T00:21:34Z

spring-kafka-test/src/main/java/org/springframework/kafka/test/rule/KafkaEmbedded.java

+//		clientVersion = AppInfoParser.getVersion();
+//		if (clientVersion.startsWith("1.1.")) {
+//			try {
+//				testUtilsCreateBrokerConfigMethod = TestUtils.class.getDeclaredMethod("createBrokerConfig",


Fair enough. Let it live as is!

- remove unnecessary excludes - add slf4j over log4j2 - add logging to -test module tests - suppress some noise from embedded kafka

artembilan · 2018-04-10T14:47:40Z

build.gradle

@@ -184,7 +188,6 @@ project ('spring-kafka') {
 		compile ("org.apache.kafka:kafka-streams:$kafkaVersion") {
 			optional it
 			exclude group: 'org.slf4j', module: 'slf4j-api'


May I know why should we still exclude this and manage our own Slf4J version?
Thanks

Given that we need to add the log4j-slf4j-impl to the classpath (to bridge from slf4j to log4j2), I think we need the api to be the same version.

The same version of what?
The log4j-slf4j-impl is a Log4J asset, not Slf4J.
What am I missing?
Thanks

Doh... pushing

artembilan · 2018-04-10T16:20:00Z

Do we still don't squash commits for this PR?
Should I include a Resolve #278 to the commit message as well?

I'll also fix StreamsBuilderFactoryBeanTests for Windows compatibility.

garyrussell · 2018-04-10T16:48:19Z

Sorry - forgot about the Windows problem. You should squash the last 3 commits; retaining 2 commits.

Yes, please add the #278 comment (to the squashed second commit).

Thanks.

artembilan · 2018-04-10T16:50:35Z

I also noticed that we have several not closed KafkaConsumers in some tests.
Fixing...
Thanks for confirmation!

artembilan · 2018-04-10T17:02:42Z

Merged as b048aaa and 9f724ea

garyrussell force-pushed the GH-550 branch from 5c3d8c7 to 1cda022 Compare April 3, 2018 21:10

garyrussell changed the title ~~GH-550: master to 2.2; fix tangles [DO NOT MERGE UNTIL 2.1.5]~~ GH-550: master to 2.2; fix tangles Apr 4, 2018

garyrussell added 2 commits April 4, 2018 13:27

spring-projectsGH-640: Upgrade kafka version to 1.1.0

176aaa0

Resolves spring-projects#640

garyrussell force-pushed the GH-550 branch from 1cda022 to 176aaa0 Compare April 4, 2018 17:27

artembilan requested changes Apr 9, 2018

View reviewed changes

artembilan requested changes Apr 10, 2018

View reviewed changes

Polishing - Logging

42d11a4

- remove unnecessary excludes - add slf4j over log4j2 - add logging to -test module tests - suppress some noise from embedded kafka

artembilan reviewed Apr 10, 2018

View reviewed changes

Remove slf4j api

a6a60e6

artembilan approved these changes Apr 10, 2018

View reviewed changes

artembilan closed this Apr 10, 2018

garyrussell deleted the GH-550 branch April 10, 2018 17:45

garyrussell mentioned this pull request Apr 11, 2018

Update LogAdjuster to Log4j2 #636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-550: master to 2.2; fix tangles #641

GH-550: master to 2.2; fix tangles #641

garyrussell commented Apr 3, 2018 •

edited

garyrussell commented Apr 3, 2018

garyrussell commented Apr 4, 2018

artembilan left a comment

artembilan Apr 9, 2018

garyrussell Apr 9, 2018

artembilan Apr 10, 2018

artembilan Apr 9, 2018

garyrussell Apr 9, 2018

garyrussell Apr 9, 2018

artembilan Apr 10, 2018

artembilan Apr 10, 2018

artembilan Apr 10, 2018

artembilan Apr 10, 2018

garyrussell Apr 10, 2018

artembilan Apr 10, 2018

garyrussell Apr 10, 2018

artembilan commented Apr 10, 2018

garyrussell commented Apr 10, 2018 •

edited

artembilan commented Apr 10, 2018

artembilan commented Apr 10, 2018

GH-550: master to 2.2; fix tangles #641

GH-550: master to 2.2; fix tangles #641

Conversation

garyrussell commented Apr 3, 2018 • edited

garyrussell commented Apr 3, 2018

garyrussell commented Apr 4, 2018

artembilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artembilan commented Apr 10, 2018

garyrussell commented Apr 10, 2018 • edited

artembilan commented Apr 10, 2018

artembilan commented Apr 10, 2018

garyrussell commented Apr 3, 2018 •

edited

garyrussell commented Apr 10, 2018 •

edited