You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "luchy0120 (GitHub)" <gi...@apache.org> on 2018/12/09 06:59:18 UTC
[GitHub] [incubator-dubbo] luchy0120 opened issue #2926: Some
problems with dubbo-cluster rpc merger
Hello, dubbo team. I'm learning the dubbo project , and I found some problems with rpc.merger module
listed as below:
1, The ArrayMerger uses its first element's component type to create a new Array. But some times
if we use
```
class Person{};
class Teacher extends Person{};
Person[] a= {new Person()};
Teacher[] b= {new Teacher()};
Object[] result = ArrayMerger.INSTANCE.merge(a,b);
```
This will fail with "array element type mismatch"
2, The ArrayMerger doesn't allow null object , but ListMerger and MapMerger allows user to input
null object
```
MergerFactory.getMerger(List.class).merge(list1, list2, null);
```
this is not semantic consistent
3, All the mergers doesn't check for null input and MergeFactory.getmerger could return null
if the request class not in cache.
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] cvictory commented on issue #2926: Some
problems with dubbo-cluster rpc merger
Posted by "cvictory (GitHub)" <gi...@apache.org>.
1, The ArrayMerger uses its first element's component type to create a new Array. But some times
if we use
```
class Person{};
class Teacher extends Person{};
Person[] a= {new Person()};
Teacher[] b= {new Teacher()};
Object[] result = ArrayMerger.INSTANCE.merge(a,b);
```
This will fail with "array element type mismatch"
------
It can work well, but it is not right.
As the source code said:
```
Class<?> type = others[0].getClass().getComponentType();
```
It will get the first param's type as parameter, I think you should keep the same type for the merge type. And it is not used by another module, so it is internal used api.
2, The ArrayMerger doesn't allow null object , but ListMerger and MapMerger allows user to input
null object
```
MergerFactory.getMerger(List.class).merge(list1, list2, null);
```
this is not semantic consistent.
---------
I think you can commit a pr to make the ArrayMerge receive the null parameter, but it still should be ignored when null parameter is inputed at the runtime.
3, All the mergers doesn't check for null input and MergeFactory.getmerger could return null
if the request class not in cache.
As above said, it is designed for internal use, so it do not have perfect logic, if you think it need be optimized, you can try to commit a pr .
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 commented on issue #2926: Some
problems with dubbo-cluster rpc merger
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
@luchy0120 pls. follow [this guide](http://dubbo.apache.org/zh-cn/docs/developers/contributor-guide/mailing-list-subscription-guide_dev.html) to subscribe to Dubbo mailing list.
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] kimmking commented on issue #2926: Some
problems with dubbo-cluster rpc merger
Posted by "kimmking (GitHub)" <gi...@apache.org>.
hi, @luchy0120 If you have any ideas for improvement, I guess you could submit a proposal via email to community and then we discuss it, complete it.
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] luchy0120 commented on issue #2926: Some
problems with dubbo-cluster rpc merger
Posted by "luchy0120 (GitHub)" <gi...@apache.org>.
@cvictory , I also think the origin one works well , but this may be a good practice for me while I'm learning and I can do something for it , thanks . By the way , is it used by Consumer or it is only internal use ? I see some blogs' description of merger, and it says it can be configured to be used by consumers to aggregate results .
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] luchy0120 commented on issue #2926: Some
problems with dubbo-cluster rpc merger
Posted by "luchy0120 (GitHub)" <gi...@apache.org>.
hi , @kimmking , I'm trying to join the email , but failed . May be I should try agian .
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] beiwei30 closed issue #2926: Some problems
with dubbo-cluster rpc merger
Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
[ issue closed by beiwei30 ]
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org
[GitHub] [incubator-dubbo] cvictory commented on issue #2926: Some
problems with dubbo-cluster rpc merger
Posted by "cvictory (GitHub)" <gi...@apache.org>.
1, The ArrayMerger uses its first element's component type to create a new Array. But some times
if we use
```
class Person{};
class Teacher extends Person{};
Person[] a= {new Person()};
Teacher[] b= {new Teacher()};
Object[] result = ArrayMerger.INSTANCE.merge(a,b);
```
This will fail with "array element type mismatch"
------
It can work well, but it is not right.
As the source code said:
```
Class<?> type = others[0].getClass().getComponentType();
```
It will get the first param's type as parameter, I think you should keep the same type for the merge type. And it is not used by another module, so it is internal used api.
2, The ArrayMerger doesn't allow null object , but ListMerger and MapMerger allows user to input
null object
```
MergerFactory.getMerger(List.class).merge(list1, list2, null);
```
this is not semantic consistent.
---------
I think you can commit a pr to make the ArrayMerge receive the null parameter, but it still should be ignored when null parameter is inputed at the runtime.
3, All the mergers doesn't check for null input and MergeFactory.getmerger could return null
if the request class not in cache.
As above said, it is designed for internal use, so it do not have perfect logic, if you think it need be optimized, you can try to commit a pr .
[ Full content available at: https://github.com/apache/incubator-dubbo/issues/2926 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org