Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[ISSUE #7231] Fix: proxy client language error#7200

Merged
RongtongJin merged 15 commits intoapache:developapache/rocketmq:developfrom
weihubeats:weihubeats/develop_proxy_language_bugfixweihubeats/rocketmq:weihubeats/develop_proxy_language_bugfixCopy head branch name to clipboard
Oct 20, 2023
Merged

[ISSUE #7231] Fix: proxy client language error#7200
RongtongJin merged 15 commits intoapache:developapache/rocketmq:developfrom
weihubeats:weihubeats/develop_proxy_language_bugfixweihubeats/rocketmq:weihubeats/develop_proxy_language_bugfixCopy head branch name to clipboard

Conversation

@weihubeats
Copy link
Copy Markdown
Member

@weihubeats weihubeats commented Aug 17, 2023 •
edited
Loading

#7231

The client actually uses go, and there is a bug in the language setting here.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 17, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 42.95%. Comparing base (82b2f8e) to head (bcd07a4).
Report is 349 commits behind head on develop.

Files with missing lines Patch % Lines
...pache/rocketmq/remoting/protocol/LanguageCode.java 50.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Aug 18, 2023 •
edited
Loading

@xdkxlk Can you help review this pr?

joeCarf
joeCarf reviewed Aug 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@joeCarf joeCarf left a 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 :)

@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Aug 21, 2023

@joeCarf All ci checks have now been cleared.

joeCarf
joeCarf reviewed Aug 21, 2023
View reviewed changes
Comment thread proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalRemotingCommand.java
Comment thread remoting/src/main/java/org/apache/rocketmq/remoting/protocol/LanguageCode.java Outdated
@weihubeats weihubeats changed the title develop proxy language bugfix [ISSUE #7199] Fix: proxy client language error Aug 21, 2023
@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Aug 21, 2023

done

joeCarf
joeCarf approved these changes Aug 21, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@joeCarf joeCarf left a comment

Choose a reason for hiding this comment

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

LGTM

@drpmma
Copy link
Copy Markdown
Contributor

drpmma commented Aug 22, 2023 •
edited
Loading

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?

java.lang.NullPointerException: null
at org.apache.rocketmq.proxy.grpc.v2.channel.GrpcClientChannel.processGetConsumerRunningInfo(GrpcClientChannel.java:213)
at org.apache.rocketmq.proxy.service.relay.ProxyChannel.writeAndFlush(ProxyChannel.java:97)
at org.apache.rocketmq.remoting.netty.NettyRemotingAbstract.invokeSyncImpl(NettyRemotingAbstract.java:476)
at org.apache.rocketmq.remoting.netty.NettyRemotingServer.invokeSync(NettyRemotingServer.java:438)
at org.apache.rocketmq.broker.client.net.Broker2Client.callClient(Broker2Client.java:80)
at org.apache.rocketmq.broker.processor.AdminBrokerProcessor.callConsumer(AdminBrokerProcessor.java:2347)
at org.apache.rocketmq.broker.processor.AdminBrokerProcessor.getConsumerRunningInfo(AdminBrokerProcessor.java:1979)
at org.apache.rocketmq.broker.processor.AdminBrokerProcessor.processRequest(AdminBrokerProcessor.java:278)
at org.apache.rocketmq.remoting.netty.NettyRemotingAbstract.lambda$buildProcessRequestHandler$1(NettyRemotingAbstract.java:312)
at org.apache.rocketmq.remoting.netty.RequestTask.run(RequestTask.java:80)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:750)

rocketmq/proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/channel/GrpcClientChannel.java

Line 213 in cc16a1b

if (!header.isJstackEnable()) {

@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Aug 22, 2023

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

@weihubeats weihubeats changed the title [ISSUE #7199] Fix: proxy client language error [ISSUE #7231] Fix: proxy client language error Aug 22, 2023
@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Aug 22, 2023

@drpmma I've changed issues, please help to relook at it

drpmma
drpmma requested changes Aug 23, 2023
View reviewed changes
Comment thread proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalRemotingCommand.java Outdated
drpmma
drpmma approved these changes Aug 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@drpmma drpmma left a comment

Choose a reason for hiding this comment

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

LGTM

@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Aug 25, 2023

@drpmma Can this pr be merged?

@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Sep 19, 2023

Is this pr ready for merge, and are there any other issues that need to be dealt with?

@RongtongJin
Copy link
Copy Markdown
Contributor

RongtongJin commented Oct 18, 2023

@weihubeats How about merging lastest code to trigger CI again？

@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Oct 18, 2023

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

@RongtongJin
Copy link
Copy Markdown
Contributor

RongtongJin commented Oct 19, 2023

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

@weihubeats
Copy link
Copy Markdown
Member Author

weihubeats commented Oct 19, 2023

Maybe this pr can be merged now. @RongtongJin

@RongtongJin RongtongJin merged commit 3968c18 into apache:develop Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drpmma drpmma drpmma approved these changes

@RongtongJin RongtongJin RongtongJin approved these changes

+1 more reviewer

@joeCarf joeCarf joeCarf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@weihubeats @codecov-commenter @drpmma @RongtongJin @joeCarf
Morty Proxy This is a proxified and sanitized view of the page, visit original site.