Skip to content

Conversation

@wxbty
Copy link
Member

@wxbty wxbty commented May 3, 2023

What is the purpose of the change

Issue Number: close #12153

Brief changelog

The MergingDigest data structure of the t-Digest package uses a cache with a default size (1050) to improve efficiency, and the cache increments by +1 each time it is added.
If the cache is full, the space will be merged and tempUsed=0 will be reset. In this process, there are common concurrency problems in the judgment and merge methods, which need to be fixed.

  1. Add synchronized to the mergeNewValues ​​method. This method will be executed about 1050 times (the reason for the 0.1% error rate), so there is no need to worry about performance issues.
  2. Add volatile modification
  3. Because the source code of t-Digest cannot be modified directly, copy the corresponding code to the dubbo source code separately
 private void add(double x, int w, List<Double> history) {
        if (Double.isNaN(x)) {
            throw new IllegalArgumentException("Cannot add NaN to t-digest");
        }
        if (tempUsed >= tempWeight.length - lastUsedCell - 1) {  //判断是否达到缓存上限,tempWeight.length =1050
            mergeNewValues();
        }
        int where = tempUsed++;    // 每次add自增
        // ...
}

image

Verifying this change

see testcase testMulti, also as follow:
1、git pull 3.2
2、modify the ConsumerApplication code of dubbo-demo-spring-boot-consumer as follows. About 0.1% error rate before the change, no error rate after the change

 public static void main(String[] args) {

        ConfigurableApplicationContext context = SpringApplication.run(ConsumerApplication.class, args);
        ConsumerApplication application = context.getBean(ConsumerApplication.class);
        String result = application.doSayHello();
        System.out.println("result: " + result);
    }

    public String doSayHello() {
        int index = 0;
        while (index < 200) {
            System.out.println("-------------:" + index);
            for (int i = 0; i < 100; i++) {
                new Thread(() -> {

                    try {
                        String result = demoService.sayHello("world");
                    } catch (Exception e) {
                        e.printStackTrace();
                    }
                }).start();

            }
            index++;
            try {
                Thread.sleep(500);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
        return "";
    }

@wxbty wxbty force-pushed the degist-concurrency-bugfix branch from 73b5bde to 1f68243 Compare May 3, 2023 08:56
@codecov-commenter
Copy link

codecov-commenter commented May 3, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.40%. Comparing base (c5f8ce6) to head (8c128c0).
⚠️ Report is 1016 commits behind head on 3.2.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.2   #12223      +/-   ##
============================================
+ Coverage     68.83%   69.40%   +0.57%     
+ Complexity      115        2     -113     
============================================
  Files          3559     1605    -1954     
  Lines        163607    66165   -97442     
  Branches      26766     9698   -17068     
============================================
- Hits         112612    45922   -66690     
+ Misses        41196    15815   -25381     
+ Partials       9799     4428    -5371     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

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

Is it possible to add some uts to verify it?

@AlbumenJ
Copy link
Member

AlbumenJ commented May 4, 2023

@songxiaosheng PTAL

@songxiaosheng
Copy link
Member

是否可以考虑比micrometer的滑动窗口函数,来实现相同的功能
Micrometer是一个度量库,它提供了许多度量指标,包括计数器、计时器、直方图等等,可以用于监控应用程序的性能和健康状况。而滑动窗口是一种常用的度量指标的实现方式,它可以用来统计一段时间内的度量指标数据,比如最近一分钟、最近一小时等等。

Micrometer提供了一个名为SlidingWindow的滑动窗口实现,可以用来统计一段时间内的度量指标数据。该实现可以通过以下方式创建:

java
Copy code
SlidingWindow slidingWindow = new SlidingWindow(TimeUnit.SECONDS, 10);

这里创建了一个10秒的滑动窗口。可以使用record方法向滑动窗口中添加度量指标数据,例如:

java
Copy code
Counter counter = Counter.builder("my.counter").register(registry);
slidingWindow.record(counter::count);

这里创建了一个名为my.counter的计数器,并将其记录到滑动窗口中。可以使用stream方法获取滑动窗口中的度量指标数据,例如:

java
Copy code
List measurements = slidingWindow.stream().map(Measurement::getValue).collect(Collectors.toList());

这里获取了滑动窗口中所有度量指标数据的值,并将其存储在一个List中。可以根据需要对这些数据进行进一步处理,例如计算平均值、最大值、最小值等等。

@wxbty wxbty requested a review from AlbumenJ May 4, 2023 11:45
@AlbumenJ
Copy link
Member

AlbumenJ commented May 5, 2023

是否可以考虑比micrometer的滑动窗口函数,来实现相同的功能 Micrometer是一个度量库,它提供了许多度量指标,包括计数器、计时器、直方图等等,可以用于监控应用程序的性能和健康状况。而滑动窗口是一种常用的度量指标的实现方式,它可以用来统计一段时间内的度量指标数据,比如最近一分钟、最近一小时等等。

Micrometer提供了一个名为SlidingWindow的滑动窗口实现,可以用来统计一段时间内的度量指标数据。该实现可以通过以下方式创建:

java Copy code SlidingWindow slidingWindow = new SlidingWindow(TimeUnit.SECONDS, 10);

这里创建了一个10秒的滑动窗口。可以使用record方法向滑动窗口中添加度量指标数据,例如:

java Copy code Counter counter = Counter.builder("my.counter").register(registry); slidingWindow.record(counter::count);

这里创建了一个名为my.counter的计数器,并将其记录到滑动窗口中。可以使用stream方法获取滑动窗口中的度量指标数据,例如:

java Copy code List measurements = slidingWindow.stream().map(Measurement::getValue).collect(Collectors.toList());

这里获取了滑动窗口中所有度量指标数据的值,并将其存储在一个List中。可以根据需要对这些数据进行进一步处理,例如计算平均值、最大值、最小值等等。

支持,我们可以在这个 PR 修复完以后尝试基于 micrometer 进行重构

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 23 Code Smells

43.1% 43.1% Coverage
0.0% 0.0% Duplication

Copy link
Member

@songxiaosheng songxiaosheng left a comment

Choose a reason for hiding this comment

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

LGTM

@songxiaosheng songxiaosheng merged commit 8b65828 into apache:3.2 May 6, 2023
@wxbty wxbty deleted the degist-concurrency-bugfix branch June 3, 2023 13:17
wxbty added a commit to wxbty/dubbo that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dubbo 3.2 turn on metrics report error ArrayIndexOutOfBoundsException

4 participants