-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ISSUE #7231] Fix: proxy client language error #7200
[ISSUE #7231] Fix: proxy client language error #7200
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7200 +/- ##
=============================================
- Coverage 42.97% 42.95% -0.03%
+ Complexity 9670 9657 -13
=============================================
Files 1161 1161
Lines 83926 83929 +3
Branches 10898 10898
=============================================
- Hits 36069 36049 -20
- Misses 43380 43394 +14
- Partials 4477 4486 +9 ☔ View full report in Codecov by Sentry. |
@xdkxlk Can you help review this pr? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are tests failures and plz give more information about this bug so I can help to review :)
@joeCarf All ci checks have now been cleared. |
proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalRemotingCommand.java
Show resolved
Hide resolved
remoting/src/main/java/org/apache/rocketmq/remoting/protocol/LanguageCode.java
Outdated
Show resolved
Hide resolved
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The issue says that there is an NPE. But I don't see a relationship between the issue and this pr. Could you please explain it?
rocketmq/proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/channel/GrpcClientChannel.java Line 213 in cc16a1b
|
There are two problems with this issues, one is null and the other is a language code problem that I can split into two issues |
@drpmma I've changed issues, please help to relook at it |
proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalRemotingCommand.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@drpmma Can this pr be merged? |
Is this pr ready for merge, and are there any other issues that need to be dealt with? |
@weihubeats How about merging lastest code to trigger CI again? |
…develop_proxy_language_bugfix
@RongtongJin I merge the latest dev bis triggered ci, but a few cid still failed, I feel that this should not be the result of this modification, this test these failed use cases can be successful! |
I will rerun. |
Maybe this pr can be merged now. @RongtongJin |
#7231
The client actually uses go, and there is a bug in the language setting here.