Feature/repairkafka3.7 #707

darkness-2nd · 2024-07-21T17:16:57Z

If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the CHANGES log.

wu-sheng · 2024-07-21T17:51:18Z

Please explain what is changing here, and if this is a plugin update, you need to update test scenarios too.

wu-sheng · 2024-07-21T23:32:46Z

If the new use cases are same here, you should add more versions to support your new implementations for newer versions, and keep compatibility with previous.

wu-sheng · 2024-07-21T23:33:45Z

...rc/main/java/org/apache/skywalking/apm/plugin/kafka/define/KafkaConsumerInstrumentation.java

+        return new ConstructorInterceptPoint[]{
+                new ConstructorInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getConstructorMatcher() {
+                        return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE);
+                    }


You should not reformat, thr code style file is in the source codes.

You should not reformat, thr code style file is in the source codes.

I reset the java file which I reformat

darkness-2nd · 2024-07-22T01:30:35Z

ase explain what is changing here, and if th

If the new use cases are same here, you should add more versions to support your new implementations for newer versions, and keep compatibility with previous.

Ok, I'll commit later

wu-sheng · 2024-07-22T05:19:53Z

About #708 (comment) I mean the test versions

demo codes https://github.com/apache/skywalking-java/tree/main/test/plugin/scenarios/kafka-scenario
tested versions https://github.com/apache/skywalking-java/blob/main/test/plugin/scenarios/kafka-scenario/support-version.list

…lid" This reverts commit 2971c2b.

…lid" This reverts commit 3feef5a.

… method's name from pollForFetches to poll

wu-sheng · 2024-07-22T07:56:49Z

Please pick one for 3.4, 3.5, 3.6 as well.

darkness-2nd · 2024-07-22T07:58:31Z

Please pick one for 3.4, 3.5, 3.6 as well.

3.4, 3.5, 3.6 are same to 3.2.3

wu-sheng · 2024-07-22T08:00:48Z

Logically same. But we should test.

darkness-2nd · 2024-07-22T08:38:16Z

Logically same. But we should test.

OK, Should I create modules kafka-3.6.x-scenario and kafka-3.7.x-scenario like kafka-2.3.x-scenario in dir /test?

wu-sheng · 2024-07-22T08:39:42Z

Logically same. But we should test.

OK, Should I create modules kafka-3.6.x-scenario and kafka-3.7.x-scenario like kafka-2.3.x-scenario in dir /test?

Those exist because the test codes are different. If you just need to change dependency versions, you don't need to do that.

wu-sheng · 2024-07-22T08:39:55Z

And please fix CIs.

… method's name from pollForFetches to poll

wu-sheng · 2024-07-23T04:52:40Z

You should verify all locally rather than costing too much on CI.

darkness-2nd · 2024-07-23T19:37:56Z

You should verify all locally rather than costing too much on CI.
Sry, it's my first time to create PR in github, I'm doing test CI in my private branch now

wu-sheng · 2024-07-25T03:21:15Z

docs/en/setup/service-agent/java-agent/Bootstrap-plugins.md

@@ -19,4 +19,3 @@ The plugin of JDK HttpURLConnection depended on `sun.net.*`. When using Java 9+,

 For more information 
 1. [JEP 403: Strongly Encapsulate JDK Internals](https://openjdk.org/jeps/403)
-2. [A peek into Java 17: Encapsulating the Java runtime internals](https://blogs.oracle.com/javamagazine/post/a-peek-into-java-17-continuing-the-drive-to-encapsulate-the-java-runtime-internals)


I think you made a mistake to remove this line?

I think you made a mistake to remove this line?

No, the CI thiks this line is a deadlink, though it can be access in browser

The blog is there. What is the deadlink check saying?

You should not remove the docs, just because a CI took says otherwise.

You should not remove the docs, just because a CI took says otherwise.

OK, I will rollback it, thank you!

wu-sheng · 2024-07-25T03:23:08Z

--- /home/runner/work/skywalking-java/skywalking-java/tools/plugin/md-javaagent-plugin-list.txt 2024-07-25 03:04:25.186078041 +0000
+++ /home/runner/work/skywalking-java/skywalking-java/tools/plugin/genernate-javaagent-plugin-list.txt 2024-07-25 03:04:25.178078015 +0000

I think you did not update docs accordingly, Plugin-list.md and Supported-list.md

wu-sheng · 2024-07-25T03:50:02Z

...rc/main/java/org/apache/skywalking/apm/plugin/kafka/define/KafkaConsumerInstrumentation.java

+                new ConstructorInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getConstructorMatcher() {
+                        return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE);
+                    }

-                @Override
-                public String getConstructorInterceptor() {
-                    return CONSUMER_CONFIG_CONSTRUCTOR_INTERCEPTOR_CLASS;
-                }
-            },
-              new ConstructorInterceptPoint() {
-                  @Override
-                  public ElementMatcher<MethodDescription> getConstructorMatcher() {
-                      return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_MAP_TYPE);
-                  }
-
-                @Override
-                public String getConstructorInterceptor() {
-                    return MAP_CONSTRUCTOR_INTERCEPTOR_CLASS;
-                }
-            },
+                    @Override
+                    public String getConstructorInterceptor() {
+                        return CONSUMER_CONFIG_CONSTRUCTOR_INTERCEPTOR_CLASS;
+                    }
+                },
+                new ConstructorInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getConstructorMatcher() {
+                        return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_MAP_TYPE);
+                    }
+
+                    @Override
+                    public String getConstructorInterceptor() {
+                        return MAP_CONSTRUCTOR_INTERCEPTOR_CLASS;
+                    }
+                },


Please revert re-format.

Please revert re-format.

I have already recerted it, this class dosen't change, I have already rollback it to the original version

I have already recerted it, this class dosen't change, I have already rollback it to the original version

No, you didn't. Change is still there.

I have already recerted it, this class dosen't change, I have already rollback it to the original version

No, you didn't. Change is still there.

Those changes are compare to the changes which are not correct I commit before...

I have already recerted it, this class dosen't change, I have already rollback it to the original version

No, you didn't. Change is still there.

I dont't know why.... I check out the original branch and copy the codes into the class file, but the IDE says that there is no difference.....

I have already recerted it, this class dosen't change, I have already rollback it to the original version

No, you didn't. Change is still there.

Resolved

wu-sheng · 2024-07-25T03:56:29Z

...-plugin/src/main/java/org/apache/skywalking/apm/plugin/kafka/Kafka37ConsumerInterceptor.java

+        Map<TopicPartition, List<ConsumerRecord<?, ?>>> rsp = new HashMap<>();
+        if (retObj instanceof ConsumerRecords) {


Why instanceof? Is there another case to have another parameter? The parent class has Map<TopicPartition, List<ConsumerRecord<?, ?>>> type as the parameter, but it should not affect this new Kafka37ConsumerInterceptor, right?

This class should be removed, I missed it, I'll delete it at next commit

darkness-2nd · 2024-07-25T05:32:44Z

--- /home/runner/work/skywalking-java/skywalking-java/tools/plugin/md-javaagent-plugin-list.txt 2024-07-25 03:04:25.186078041 +0000
+++ /home/runner/work/skywalking-java/skywalking-java/tools/plugin/genernate-javaagent-plugin-list.txt 2024-07-25 03:04:25.178078015 +0000

I think you did not update docs accordingly, Plugin-list.md and Supported-list.md

And please fix CIs.

All the CIs were successfully executed but the DeadLink check

wu-sheng · 2024-07-25T05:32:47Z

@kezhenxu94 As 403 respond code, should we add this into ignore list?

lujiajing1126 · 2024-07-25T05:37:53Z

...rc/main/java/org/apache/skywalking/apm/plugin/kafka/define/KafkaConsumerInstrumentation.java

+                new ConstructorInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getConstructorMatcher() {
+                        return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_TYPE);
+                    }

-                @Override
-                public String getConstructorInterceptor() {
-                    return CONSUMER_CONFIG_CONSTRUCTOR_INTERCEPTOR_CLASS;
-                }
-            },
-              new ConstructorInterceptPoint() {
-                  @Override
-                  public ElementMatcher<MethodDescription> getConstructorMatcher() {
-                      return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_MAP_TYPE);
-                  }
-
-                @Override
-                public String getConstructorInterceptor() {
-                    return MAP_CONSTRUCTOR_INTERCEPTOR_CLASS;
-                }
-            },
+                    @Override
+                    public String getConstructorInterceptor() {
+                        return CONSUMER_CONFIG_CONSTRUCTOR_INTERCEPTOR_CLASS;
+                    }
+                },
+                new ConstructorInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getConstructorMatcher() {
+                        return takesArgumentWithType(0, CONSTRUCTOR_INTERCEPT_MAP_TYPE);
+                    }
+
+                    @Override
+                    public String getConstructorInterceptor() {
+                        return MAP_CONSTRUCTOR_INTERCEPTOR_CLASS;
+                    }
+                },


I have already recerted it, this class dosen't change, I have already rollback it to the original version

No, you didn't. Change is still there.

lujiajing1126 · 2024-07-25T05:41:05Z

.../java/org/apache/skywalking/apm/plugin/kafka/define/Kafka37AsyncConsumerInstrumentation.java

+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName;
+
+/**


For the comment, I think we can focus on the difference instead of repeating what is already written in the parent class.

For the comment, I think we can focus on the difference instead of repeating what is already written in the parent class.

Yes, the difference is the method named pollForFetchs was removed from KafkaConsumer to another two classes, so the original interceptor can not intercept it. Because of the enhance class is changed, so I create two new classes to repair the uncompatible problem.

Yes, the difference is the method named pollForFetchs was removed from KafkaConsumer to another two classes, so the original interceptor can not intercept it. Because of the enhance class is changed, so I create two new classes to repair the uncompatible problem.

I meant you can modify the comment for this class......

You are right, I forget to change the comment

…cepted

wu-sheng · 2024-07-25T07:15:31Z

One more. changes.md should be updated as well.

darkness-2nd · 2024-07-25T07:19:33Z

One more. changes.md should be updated as well.
Changed

docs/en/setup/service-agent/java-agent/Supported-list.md

darkness-2nd added 2 commits July 22, 2024 00:38

Fixed an issue where intercept methods for kafka3.7 were invalid

3feef5a

Fixed an issue where intercept methods for kafka3.7 were invalid

2971c2b

wu-sheng reviewed Jul 21, 2024

View reviewed changes

wu-sheng added enhancement New feature or request plugin labels Jul 21, 2024

darkness-2nd added 3 commits July 22, 2024 15:46

Revert "Fixed an issue where intercept methods for kafka3.7 were inva…

7db4f51

…lid" This reverts commit 2971c2b.

Revert "Fixed an issue where intercept methods for kafka3.7 were inva…

1cbc3db

…lid" This reverts commit 3feef5a.

add support for kafka3.7.x, because kafka3.7 changed the pull message…

6220bd4

… method's name from pollForFetches to poll

darkness-2nd added 6 commits July 23, 2024 09:41

add support for kafka3.7.x, because kafka3.7 changed the pull message…

1e2b82a

… method's name from pollForFetches to poll

add support for kafka3.7.x, because kafka3.7 changed the pull message…

d875bd3

… method's name from pollForFetches to poll

add support for kafka3.7.x, because kafka3.7 changed the pull message…

ef2193b

… method's name from pollForFetches to poll

repair ci error

e20ef76

add case of 3.6.0

3543204

fix upe

526abbb

darkness-2nd added 4 commits July 25, 2024 10:20

repair spring-kafka 2.2.x ci

abd3848

remove dead line in Bootstrap-plugins.md

0cb524c

repair the uncompatible code in kafka Case

f31fb6f

test ci for 3.7 jdk ci

8297b83

wu-sheng reviewed Jul 25, 2024

View reviewed changes

darkness-2nd added 2 commits July 25, 2024 11:36

add support version in Supported-list.md

73c85a0

remove error class

1cc9438

wu-sheng reviewed Jul 25, 2024

View reviewed changes

remove error class

960be6c

lujiajing1126 reviewed Jul 25, 2024

View reviewed changes

darkness-2nd and others added 3 commits July 25, 2024 13:59

Add comments for resolve the problem that Kafka3.7.x can not be inter…

14ebf37

…cepted

revert the format codes

39353e8

ignore 403 when checking markdown link

3f51753

wu-sheng previously approved these changes Jul 25, 2024

View reviewed changes

Update CHANGES.md: Support kafka-clients-3.7.x intercept

1a5f39c

darkness-2nd dismissed wu-sheng’s stale review via 1a5f39c July 25, 2024 07:19

wu-sheng added this to the 9.3.0 milestone Jul 25, 2024

wu-sheng reviewed Jul 25, 2024

View reviewed changes

docs/en/setup/service-agent/java-agent/Supported-list.md Outdated Show resolved Hide resolved

Update docs/en/setup/service-agent/java-agent/Supported-list.md

b876765

wu-sheng approved these changes Jul 25, 2024

View reviewed changes

wu-sheng merged commit 744eb1e into apache:main Jul 25, 2024
190 of 191 checks passed

		Map<TopicPartition, List<ConsumerRecord<?, ?>>> rsp = new HashMap<>();
		if (retObj instanceof ConsumerRecords) {

Feature/repairkafka3.7 #707

Feature/repairkafka3.7 #707

Conversation

darkness-2nd commented Jul 21, 2024 • edited Loading

wu-sheng commented Jul 21, 2024

wu-sheng commented Jul 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkness-2nd commented Jul 22, 2024

wu-sheng commented Jul 22, 2024

wu-sheng commented Jul 22, 2024

darkness-2nd commented Jul 22, 2024

wu-sheng commented Jul 22, 2024

darkness-2nd commented Jul 22, 2024

wu-sheng commented Jul 22, 2024

wu-sheng commented Jul 22, 2024

wu-sheng commented Jul 23, 2024

darkness-2nd commented Jul 23, 2024

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

wu-sheng commented Jul 25, 2024

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

darkness-2nd commented Jul 25, 2024

wu-sheng commented Jul 25, 2024

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

wu-sheng commented Jul 25, 2024

darkness-2nd commented Jul 25, 2024

darkness-2nd commented Jul 21, 2024 •

edited

Loading