matanper opened a new issue, #70524: URL: https://github.com/apache/doris/issues/70524
### Search before asking - [x] I had searched in the [issues](https://github.com/apache/doris/issues?q=is%3Aissue) and found no similar issues. ### Version 4.1.4. master (`e125ea85867`) has the same code. ### What's Wrong? In cloud mode, a statement occasionally fails before it executes with `ConcurrentModificationException` thrown from FE metrics code: ``` java.util.ConcurrentModificationException at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1013) at java.util.ArrayList$Itr.next(ArrayList.java:967) at org.apache.doris.metric.DorisMetricRegistry.computeLabelId(DorisMetricRegistry.java:134) at org.apache.doris.metric.DorisMetricRegistry.addMetrics(DorisMetricRegistry.java:47) at org.apache.doris.metric.MetricRepo.increaseClusterRequestAll(MetricRepo.java:2021) at org.apache.doris.qe.ConnectProcessor.executeQuery(ConnectProcessor.java:275) at org.apache.doris.qe.ConnectProcessor.handleQuery(ConnectProcessor.java:245) ``` Over Arrow Flight the client gets gRPC INTERNAL `error msg: ConcurrentModificationException`. It is rare per statement, but it hits any statement type, it happens on every FE, and it gets more frequent with statement concurrency. We see it on arm64 FEs. ### Why `ConnectProcessor.executeQuery` counts every statement per compute group through `MetricRepo.increaseClusterRequestAll` (`MetricRepo.java:2179-2195` on master): ```java LongCounterMetric counter = CloudMetrics.CLUSTER_REQUEST_ALL_COUNTER.getOrAdd(clusterId); List<MetricLabel> labels = new ArrayList<>(); counter.increase(1L); labels.add(new MetricLabel("cluster_id", clusterId)); labels.add(new MetricLabel("cluster_name", clusterName)); counter.setLabels(labels); MetricRepo.DORIS_METRIC_REGISTER.addMetrics(counter); ``` - The counter is shared by all statements on that compute group, and every call replaces its label list. - `Metric.setLabels` assigns the new list to a plain (non-volatile) field, and `Metric.addLabel` adds to the current list in place. - `DorisMetricRegistry.addMetrics` → `computeLabelId` iterates `metric.getLabels()`. So do the Prometheus and JSON metric visitors. - A list published through a non-volatile field can be observed half-constructed by another thread. On a weakly ordered CPU (arm64) the iterator then sees `modCount` change during iteration and throws. `addLabel` on a published list throws for any reader on any CPU. The only exception caught around the accounting in `ConnectProcessor.executeQuery` is `ComputeGroupException`, so the metrics failure fails the statement. `increaseClusterQueryAll`, `increaseClusterQueryErr`, the warm-up job counters and the per-second gauges use the same pattern. ### What You Expected? Metric accounting never fails a statement. A metric's label list can be replaced while it is read. ### How to Reproduce? A cloud-mode cluster with an arm64 FE, and many concurrent short statements on one compute group (for example several clients issuing `SET` + `SELECT` over Arrow Flight). Grep fe.warn for `statement failed before execution` followed by `ConcurrentModificationException` in `computeLabelId`. A unit test reproduces it deterministically: writer threads call `setLabels` / `addLabel` on one `LongCounterMetric` while reader threads iterate `getLabels()` and call `DorisMetricRegistry.addMetrics(counter)`. ### Anything Else? Proposed fix: - `Metric`: never modify a label list after publishing it. Make `labels` volatile, have `setLabels` store a copy, and have `addLabel` (synchronized) install a new list instead of adding to the current one. Readers need no change. - `ConnectProcessor.executeQuery`: also catch `RuntimeException` around the request accounting and log it, so a metrics failure cannot fail the statement it counts. - Optionally, set a per-cluster counter's labels once when the counter is created, instead of on every call. We have a patch with a unit test and can open a PR. ### Are you willing to submit PR? - [x] Yes I am willing to submit a PR! ### Code of Conduct - [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
