You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/10/12 06:25:59 UTC
[GitHub] [skywalking-java] lujiajing1126 opened a new pull request #50: Fix version compatibility for JSON-RPC4J Plugin
lujiajing1126 opened a new pull request #50:
URL: https://github.com/apache/skywalking-java/pull/50
Signed-off-by: Megrez Lu <lu...@gmail.com>
Background: This plugin has been introduced by @gy09535 in the PR https://github.com/apache/skywalking/pull/6743. He was working in our team at that time and this plugin was extracted from our own codebase. Recently I've noticed a version incompatibility with early versions of jsonrpc4j, for example, all versions `<1.5.0`.
The root cause of this bug is that we actually impose much too strong constraint on the constructor matcher,
```java
@Override
public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
return new ConstructorInterceptPoint[]{
new ConstructorInterceptPoint() {
@Override
public ElementMatcher<MethodDescription> getConstructorMatcher() {
return ElementMatchers.takesArguments(6).and(ElementMatchers.takesArgument(1, URL.class));
}
@Override
public String getConstructorInterceptor() {
return INTERCEPTOR_CLASS;
}
},
new ConstructorInterceptPoint() {
@Override
public ElementMatcher<MethodDescription> getConstructorMatcher() {
return ElementMatchers.takesArguments(5).and(ElementMatchers.takesArgument(1, URL.class));
}
@Override
public String getConstructorInterceptor() {
return INTERCEPTOR_CLASS;
}
}
};
}
```
neither of the constructors exists in versions below `1.5.0`.
Also, I suppose there is problem with test configuration: we have to use `<test.framework.version>`.
### Fix backwards compatibility for jsonrpc4j plugin
- [ ] Reproduce this issue in CI.
- [ ] Add a unit test to verify that the fix works.
- [x] Explain briefly why the bug exists and how to fix it.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #50: Fix version compatibility for JSON-RPC4J Plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #50:
URL: https://github.com/apache/skywalking-java/pull/50
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #50: Fix version compatibility for JSON-RPC4J Plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #50:
URL: https://github.com/apache/skywalking-java/pull/50#discussion_r726807647
##########
File path: test/plugin/scenarios/jsonrpc4j-1.x-scenario/pom.xml
##########
@@ -57,7 +57,7 @@
<dependency>
<groupId>com.github.briandilley.jsonrpc4j</groupId>
<artifactId>jsonrpc4j</artifactId>
- <version>${jsonrpc.version}</version>
+ <version>${test.framework.version}</version>
Review comment:
This is correct fix.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on pull request #50: Fix version compatibility for JSON-RPC4J Plugin
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on pull request #50:
URL: https://github.com/apache/skywalking-java/pull/50#issuecomment-940714468
> > Recently I've noticed a version incompatibility with early versions of jsonrpc4j, for example, all versions <1.5.0.
>
> The test case fails after you corrected the test codes for `1.4.6`.
>
> @lujiajing1126 Are you going to continue on this PR to fix this bug?
Yes. I will fix this soon.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on pull request #50: Fix version compatibility for JSON-RPC4J Plugin
Posted by GitBox <gi...@apache.org>.
lujiajing1126 commented on pull request #50:
URL: https://github.com/apache/skywalking-java/pull/50#issuecomment-940823845
@wu-sheng Done. Pls have a review.
After this PR, we can guarantee all supported versions from `1.2.0` to the latest `1.6`.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #50: Fix version compatibility for JSON-RPC4J Plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #50:
URL: https://github.com/apache/skywalking-java/pull/50#issuecomment-940714042
> Recently I've noticed a version incompatibility with early versions of jsonrpc4j, for example, all versions <1.5.0.
The test case fails after you corrected the test codes for `1.4.6`.
@lujiajing1126 Are you going to continue on this PR to fix this bug?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org