Doris-Breakwater commented on issue #70524: URL: https://github.com/apache/doris/issues/70524#issuecomment-6824278286
Breakwater-GitHub-Analysis-Slot: slot_fcf6845b075b This is actionable as an FE/cloud metrics concurrency defect with statement-availability impact. The proposed snapshot-based fix is well motivated. The unsafe label API and the pre-execution exception escape are verified in source; the precise interleaving responsible for the reported arm64 failures remains to be demonstrated. Please open the proposed PR with the reproducer and tests. I inspected tag `4.1.4` (commit `ad35a140c7fd0b842f18c23300bac581f7d04326`) and the reported master revision `e125ea858678fd2aea5ebf179359650aa77fabb6`, without changing repository files or branches. The relevant `Metric` implementation is identical in both. Verified code behavior: - [`AutoMappedMetric.getOrAdd`](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/fe/fe-core/src/main/java/org/apache/doris/metric/AutoMappedMetric.java#L33) uses `ConcurrentHashMap.computeIfAbsent`. Statements sharing a compute-group ID therefore obtain the same counter. This protects map operations, not subsequent modifications to the counter's labels. - [`MetricRepo.increaseClusterRequestAll`](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/fe/fe-core/src/main/java/org/apache/doris/metric/MetricRepo.java#L2179) constructs a fresh two-label list, replaces the shared counter's labels, and registers the counter on every call. Query/error counters and warm-up counters use the same pattern; `MetricCalculator` also passes counter labels to the per-second gauges. - [`Metric`](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/fe/fe-core/src/main/java/org/apache/doris/metric/Metric.java#L49) has a non-volatile label reference, aliases the caller's list in `setLabels`, exposes it through `getLabels`, and mutates it in place in `addLabel`. [`DorisMetricRegistry.addMetrics`](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/fe/fe-core/src/main/java/org/apache/doris/metric/DorisMetricRegistry.java#L42) computes the label ID before entering the synchronized `MetricList.addMetrics`; that synchronization does not protect iteration of the labels. Prometheus and JSON visitors read the same exposed lists. - [`ConnectProcessor.executeQuery`](https://github.com/apache/doris/blob/e125ea858678fd2aea5ebf179359650aa77fabb6/fe/fe-core/src/main/java/org/apache/doris/qe/ConnectProcessor.java#L252) performs request accounting before SQL conversion, parsing, or executor creation. Its local catch only handles `ComputeGroupException`, so a metrics `ConcurrentModificationException` exits this method before statement execution. This explains why the failure can affect different SQL statement types rather than a particular execution operator. Two distinctions matter for root-cause confidence: - Replacing the label-list reference alone does **not** structurally modify the list already held by an iterator. Concurrent `addLabel`, or modification through an aliased input/output list, is a direct invalidation mechanism. The inspected request-accounting path builds its list before assignment and does not itself call `addLabel` on that counter. Unsafe publication remains a plausible explanation for inconsistent visibility of list state, supported by the [Java memory-model rules](https://docs.oracle.com/javase/specs/jls/se17/html/jls-17.html#jls-17.4.5), but the supplied stack does not establish the exact writer/interleaving. This is not demonstrated to be arm64-exclusive. - A mixed `setLabels`/`addLabel` test can establish the API defect without proving that the same mutation occurs in production. Also, the inspected `handleQuery` has an outer generic-exception catch that ignores the exception; the exact gRPC INTERNAL conversion and deployed logging behavior need the complete FE stack/build information. Aborting `executeQuery` is source-verified; the precise client response is not independently reproduced here. For the PR, I suggest: 1. Publish labels as safely visible, immutable snapshots: a volatile reference, a defensive copy in `setLabels`, and copy-on-write `addLabel`. Keep the initial list and lists returned by `getLabels` read-only, and audit subclass access to the protected field. Define ordering between concurrent replacement and addition; if writer serialization is intended, both mutators must participate in the same locking scheme. Preserve existing duplicate-label behavior. 2. Keep runtime-exception handling narrowly around metrics accounting, with diagnostic logging, so accounting failure cannot prevent execution. Do not broaden this to suppress SQL execution errors or JVM `Error`s. Treat this as containment alongside the label fix. 3. Validate retained-snapshot stability during replacement/addition, input-list alias isolation, registry registration, and both metric visitors. Use a controlled iterator-before-mutation test for the in-place defect; keep a separate replacement-only concurrent stress test for the reported publication hypothesis. Inject an accounting exception and verify that the statement actually executes and returns the correct result over Arrow Flight and MySQL, while ordinary statement failures still surface. 4. Treat initialization-only labeling as a separate optimization. Check compute-group rename, drop/recreation, and registry cleanup before making labels permanent. Existing `HistogramMetric` already provides a defensive-copy/read-only-list precedent, though its labels are fixed at construction. Please include the proposed patch/test, the exact deployed FE build hash, JVM vendor/version/options and OS/CPU details, one complete `fe.warn` exception with surrounding pre-execution log lines, and a minimal client script with concurrency and approximate failure rate. These are needed to confirm the production interleaving and client-error path; a query profile is not a prerequisite for a failure before parsing. This investigation performed source inspection only and did not run the proposed test or reproduce the production failure. Once validated, maintainers can assess backporting to the affected release branch. -- 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]
