apache / dubbo

The java implementation of Apache Dubbo. An RPC and microservice framework.
https://dubbo.apache.org/
Apache License 2.0
40.52k stars 26.43k forks source link

Filter Order was disrupted when user application extends the SPI of org.apache.dubbo.rpc.Filter #7757

Closed PromiseChan closed 3 years ago

PromiseChan commented 3 years ago

Environment

Steps to reproduce this issue

  1. Define the SPI extension point of org.apache.dubbo.rpc.Filter image

  2. write the filter and dubbo service (1)filter image (2)dubbo service image

  3. start the dubbo provider application image

Expected Result

ExecuteLimitFilter -> MonitorFilter (the picture below is sorted Filters with only dubbo Native Filler) image

Actual Result

MonitorFilter -> ExecuteLimitFilter Causes the concurring count to be inaccurate !

image

What actually happens? the Filters were sorted in the TreeMap , using ActivateComparator. When the order of Flter is not set, the traversal order will affect the sort results. image

xiaoheng1 commented 3 years ago

I think this should not affect it. If the order is not set, it means that the user actually doesn't care about the execution order of this filter.

PromiseChan commented 3 years ago

I think this should not affect it. If the order is not set, it means that the user actually doesn't care about the execution order of this filter.

Of course, if user's extension filter does't set order, we can think of it as the user doesn't care about the order of user filter's execution. However, the default order of dubbo's own filter,for example, ExecuteLimitFilter and MonitorFilter ,the order between them, is disrupted. MonitorFilter should record the number of concurres that actually execute the method, when the order is disrupted, it turned out to record the request concurres. The number of concurrency in the monitoring data will be greater than the threshold. This will cause confusion to users, whether the concurrency settings of the service take effect.

xiaoheng1 commented 3 years ago

Of course, if user's extension filter does't set order, we can think of it as the user doesn't care about the order of user filter's execution. However, the default order of dubbo's own filter,for example, ExecuteLimitFilter and MonitorFilter ,the order between them, is disrupted. MonitorFilter should record the number of concurres that actually execute the method, when the order is disrupted, it turned out to record the request concurres. The number of concurrency in the monitoring data will be greater than the threshold. This will cause confusion to users, whether the concurrency settings of the service take effect.

This may be a problem. That is, whether the filter that comes with the system is equivalent to the user-defined filter.

xiaoheng1 commented 3 years ago

If they are equivalent, then since MonitorFilter and ExecuteLimitFilter do not define an order, they do not care about the order of execution.

xiaoheng1 commented 3 years ago

@PromiseChan if the user-defined filter is inserted into the system-defined filter, will this affect the result? My understanding is that the filter is in order to do some pre-filtering. If the filter has a strong consistent relationship, it should be specified during configuration. If not, there is no need for the order of relationships, do you think so?

PromiseChan commented 3 years ago

@PromiseChan if the user-defined filter is inserted into the system-defined filter, will this affect the result? My understanding is that the filter is in order to do some pre-filtering. If the filter has a strong consistent relationship, it should be specified during configuration. If not, there is no need for the order of relationships, do you think so?

Yes, i do. If users want an order between filters, they should make configuration on filers. By the way, the result was not affected with unordered pre-filtering filters. Just the meaning of monitoring data has changed.

xiaoheng1 commented 3 years ago

ok

@PromiseChan if the user-defined filter is inserted into the system-defined filter, will this affect the result? My understanding is that the filter is in order to do some pre-filtering. If the filter has a strong consistent relationship, it should be specified during configuration. If not, there is no need for the order of relationships, do you think so?

Yes, i do. If users want an order between filters, they should make configuration on filers. By the way, the result was not affected with unordered pre-filtering filters. Just the meaning of monitoring data has changed.

The monitoring time you mentioned by monitor data has changed, right?

PromiseChan commented 3 years ago

ok

@PromiseChan if the user-defined filter is inserted into the system-defined filter, will this affect the result? My understanding is that the filter is in order to do some pre-filtering. If the filter has a strong consistent relationship, it should be specified during configuration. If not, there is no need for the order of relationships, do you think so?

Yes, i do. If users want an order between filters, they should make configuration on filers. By the way, the result was not affected with unordered pre-filtering filters. Just the meaning of monitoring data has changed.

The monitoring time you mentioned by monitor data has changed, right?

monitoring time? I mean the method's concurrent number collected by MonitorFilter is incorrect when the filter order is monitorFiler -> executeLimitFilter

PromiseChan commented 3 years ago

ok

@PromiseChan if the user-defined filter is inserted into the system-defined filter, will this affect the result? My understanding is that the filter is in order to do some pre-filtering. If the filter has a strong consistent relationship, it should be specified during configuration. If not, there is no need for the order of relationships, do you think so?

Yes, i do. If users want an order between filters, they should make configuration on filers. By the way, the result was not affected with unordered pre-filtering filters. Just the meaning of monitoring data has changed.

The monitoring time you mentioned by monitor data has changed, right?

monitoring time? I mean the method's concurrent number collected by MonitorFilter is incorrect when the filter order is monitorFiler -> executeLimitFilter

First, I set the executes=10 , only system-defined filters, the recorded max method concurrent number is 10 . Second, I set the executes=10 with some user-defined filters , the recorded max method concurrent number is 17, which is exceeded the threshold 10.

xiaoheng1 commented 3 years ago

If this is the case, it indicates that the user needs to set the number of concurrency to 17, so I think this is ok.

PromiseChan commented 3 years ago

If this is the case, it indicates that the user needs to set the number of concurrency to 17, so I think this is ok.

Maybe I didn't make it clear... Case 1 : run 20 threads to invoke dubbo service A (only system-defined filters) , set executes=10 on service A Case 2: run 20 threads to invoke dubbo service A (system-defined filters + user-defined filters) , set executes=10 on service A

We use dubbo-monitor-simple project to monitor services. we find that even if both cases have set executes=10, the dubbo-monitor displays the concurrent number is 17 in case 2. In case 1, the concurrent number is 10. At first we think the settings , "executes=10" , didn't work in case 2. Finally we find that the order between ExecuteLimitFilter and MonitorFiler affects the count of concurrent number

xiaoheng1 commented 3 years ago

In case two, did the user-defined filter tamper with the value of executes?

PromiseChan commented 3 years ago

In case two, did the user-defined filter tamper with the value of executes?

no , the value of executes in ExecuteLimitFiler is still 10. Just in dubbo-monitor-simple project, the number displays as 17.

xiaoheng1 commented 3 years ago

In case two, did the user-defined filter tamper with the value of executes?

no , the value of executes in ExecuteLimitFiler is still 10. Just in dubbo-monitor-simple project, the number displays as 17.

Which indicator are you focusing on in the monitoring center? concurrent this?

PromiseChan commented 3 years ago

In case two, did the user-defined filter tamper with the value of executes?

no , the value of executes in ExecuteLimitFiler is still 10. Just in dubbo-monitor-simple project, the number displays as 17.

Which indicator are you focusing on in the monitoring center? concurrent this?

yes, the concurrent

xiaoheng1 commented 3 years ago

I probably understand the problem you described, let me think about it.