You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2022/01/29 14:59:19 UTC
[GitHub] [dolphinscheduler] krystalics opened a new pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
krystalics opened a new pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263
something detail is in [Bug] [remote-rpc] the asynchronous call's method can't use other return type except Boolean #8262
https://github.com/apache/dolphinscheduler/issues/8262
in a summary. I add a test mehod which return string,but happend a error
so i fix it.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1028588998
> > It seems this pr doesn't solve the callback issue? When we use an async request, we hope the callback function can be trigger when the result return.
>
> it's already implemented by NettyClientHandler,when the result return by channel with netty, it will trigger the channelRead() and then call the readHandler() to judge the method is async or not. the original code is below
>
> ```java
> private void readHandler(RpcResponse rsp, RpcRequestCache rpcRequest, long reqId) {
> String serviceName = rpcRequest.getServiceName();
> ConsumerConfig consumerConfig = ConsumerConfigCache.getConfigByServersName(serviceName);
> if (Boolean.FALSE.equals(consumerConfig.getAsync())) {
> RpcFuture future = rpcRequest.getRpcFuture();
> RpcRequestTable.remove(reqId);
> future.done(rsp);
> return;
> }
>
> if (Boolean.FALSE.equals(consumerConfig.getCallBack())) {
> return;
> }
>
> if (rsp.getStatus() == 0) {
>
> try {
> consumerConfig.getServiceCallBackClass().getDeclaredConstructor().newInstance().run(rsp.getResult());
> } catch (InstantiationException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
> logger.error("rpc service call back error,serviceName {},rsp {}", serviceName, rsp);
> }
> } else {
> logger.error("rpc response error ,serviceName {},rsp {}", serviceName, rsp);
> }
>
> }
> ```
Yes, I see, this can work well now.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1028582016
> > It seems this pr doesn't solve the callback issue? When we use an async request, we hope the callback function can be trigger when the result return.
>
> it's already implemented by NettyClientHandler,when the result return by channel with netty, it will trigger the channelRead() and then call the readHandler() to judge the method is async or not. the original code is below
>
> ```java
> private void readHandler(RpcResponse rsp, RpcRequestCache rpcRequest, long reqId) {
> String serviceName = rpcRequest.getServiceName();
> ConsumerConfig consumerConfig = ConsumerConfigCache.getConfigByServersName(serviceName);
> if (Boolean.FALSE.equals(consumerConfig.getAsync())) {
> RpcFuture future = rpcRequest.getRpcFuture();
> RpcRequestTable.remove(reqId);
> future.done(rsp);
> return;
> }
>
> if (Boolean.FALSE.equals(consumerConfig.getCallBack())) {
> return;
> }
>
> if (rsp.getStatus() == 0) {
>
> try {
> consumerConfig.getServiceCallBackClass().getDeclaredConstructor().newInstance().run(rsp.getResult());
> } catch (InstantiationException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
> logger.error("rpc service call back error,serviceName {},rsp {}", serviceName, rsp);
> }
> } else {
> logger.error("rpc response error ,serviceName {},rsp {}", serviceName, rsp);
> }
>
> }
> ```
It seems this cannot work success.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] krystalics commented on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
krystalics commented on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1027825624
> It seems this pr doesn't solve the callback issue? When we use an async request, we hope the callback function can be trigger when the result return.
it's already implemented by NettyClientHandler,when the result return by channel with netty, it will trigger the channelRead() and then call the readHandler() to judge the method is async or not.
the original code is below
```java
private void readHandler(RpcResponse rsp, RpcRequestCache rpcRequest, long reqId) {
String serviceName = rpcRequest.getServiceName();
ConsumerConfig consumerConfig = ConsumerConfigCache.getConfigByServersName(serviceName);
if (Boolean.FALSE.equals(consumerConfig.getAsync())) {
RpcFuture future = rpcRequest.getRpcFuture();
RpcRequestTable.remove(reqId);
future.done(rsp);
return;
}
if (Boolean.FALSE.equals(consumerConfig.getCallBack())) {
return;
}
if (rsp.getStatus() == 0) {
try {
consumerConfig.getServiceCallBackClass().getDeclaredConstructor().newInstance().run(rsp.getResult());
} catch (InstantiationException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
logger.error("rpc service call back error,serviceName {},rsp {}", serviceName, rsp);
}
} else {
logger.error("rpc response error ,serviceName {},rsp {}", serviceName, rsp);
}
}
```
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1028600061
SonarCloud Quality Gate failed. 
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=8263&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=8263&metric=new_coverage&view=list) [5.3% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=8263&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=8263&metric=new_duplicated_lines_density&view=list) [3.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=8263&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] krystalics commented on a change in pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
krystalics commented on a change in pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#discussion_r795116196
##########
File path: dolphinscheduler-remote/src/main/java/org/apache/dolphinscheduler/rpc/remote/NettyClient.java
##########
@@ -211,7 +211,8 @@ public RpcResponse sendMsg(Host host, RpcProtocol<RpcRequest> protocol, Boolean
if (Boolean.TRUE.equals(async)) {
result = new RpcResponse();
result.setStatus((byte) 0);
- result.setResult(true);
+ //firstly return a null value when meet the async call
+ result.setResult(null);
Review comment:
in my opinion , the return type of meothd in async call cannot used directly,it must use the callback mechanism to get the real response.
so the `result==true` can't be judge in the very fisrt time to use.
It's very awesome you can reply in this Spring Festival , thx
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] caishunfeng commented on a change in pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
caishunfeng commented on a change in pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#discussion_r795114697
##########
File path: dolphinscheduler-remote/src/main/java/org/apache/dolphinscheduler/rpc/remote/NettyClient.java
##########
@@ -211,7 +211,8 @@ public RpcResponse sendMsg(Host host, RpcProtocol<RpcRequest> protocol, Boolean
if (Boolean.TRUE.equals(async)) {
result = new RpcResponse();
result.setStatus((byte) 0);
- result.setResult(true);
+ //firstly return a null value when meet the async call
+ result.setResult(null);
Review comment:
It seems not the general processing if others need to judge `result==true`
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1028581772
>
It seems this can not work success.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun removed a comment on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
ruanwenjun removed a comment on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1028581772
>
It seems this can not work success.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] codecov-commenter commented on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1028595791
# [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/8263?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8263](https://codecov.io/gh/apache/dolphinscheduler/pull/8263?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e8c1a8) into [dev](https://codecov.io/gh/apache/dolphinscheduler/commit/01936a660e14bfc6ea061bec065e47f4541b7519?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (01936a6) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/dolphinscheduler/pull/8263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## dev #8263 +/- ##
============================================
+ Coverage 45.14% 45.16% +0.01%
- Complexity 3979 3981 +2
============================================
Files 678 678
Lines 26343 26343
Branches 2836 2836
============================================
+ Hits 11893 11897 +4
+ Misses 13329 13327 -2
+ Partials 1121 1119 -2
```
| [Impacted Files](https://codecov.io/gh/apache/dolphinscheduler/pull/8263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...pache/dolphinscheduler/rpc/remote/NettyClient.java](https://codecov.io/gh/apache/dolphinscheduler/pull/8263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1yZW1vdGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcnBjL3JlbW90ZS9OZXR0eUNsaWVudC5qYXZh) | `77.41% <100.00%> (ø)` | |
| [...e/dolphinscheduler/remote/NettyRemotingClient.java](https://codecov.io/gh/apache/dolphinscheduler/pull/8263/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1yZW1vdGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvcmVtb3RlL05ldHR5UmVtb3RpbmdDbGllbnQuamF2YQ==) | `53.52% <0.00%> (+2.81%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/8263?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/8263?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [01936a6...3e8c1a8](https://codecov.io/gh/apache/dolphinscheduler/pull/8263?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun removed a comment on pull request #8263: bugfix:[Bug] [remote-rpc] the asynchronous call method cannot use oth…
Posted by GitBox <gi...@apache.org>.
ruanwenjun removed a comment on pull request #8263:
URL: https://github.com/apache/dolphinscheduler/pull/8263#issuecomment-1028582016
> > It seems this pr doesn't solve the callback issue? When we use an async request, we hope the callback function can be trigger when the result return.
>
> it's already implemented by NettyClientHandler,when the result return by channel with netty, it will trigger the channelRead() and then call the readHandler() to judge the method is async or not. the original code is below
>
> ```java
> private void readHandler(RpcResponse rsp, RpcRequestCache rpcRequest, long reqId) {
> String serviceName = rpcRequest.getServiceName();
> ConsumerConfig consumerConfig = ConsumerConfigCache.getConfigByServersName(serviceName);
> if (Boolean.FALSE.equals(consumerConfig.getAsync())) {
> RpcFuture future = rpcRequest.getRpcFuture();
> RpcRequestTable.remove(reqId);
> future.done(rsp);
> return;
> }
>
> if (Boolean.FALSE.equals(consumerConfig.getCallBack())) {
> return;
> }
>
> if (rsp.getStatus() == 0) {
>
> try {
> consumerConfig.getServiceCallBackClass().getDeclaredConstructor().newInstance().run(rsp.getResult());
> } catch (InstantiationException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
> logger.error("rpc service call back error,serviceName {},rsp {}", serviceName, rsp);
> }
> } else {
> logger.error("rpc response error ,serviceName {},rsp {}", serviceName, rsp);
> }
>
> }
> ```
It seems this cannot work success.
--
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: commits-unsubscribe@dolphinscheduler.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org