wenzhenghu commented on PR #65574: URL: https://github.com/apache/doris/pull/65574#issuecomment-5021759915
Review conclusion: Request changes. Reviewed at head `b827f9894817bc6fa0dcea8d0b5e91ac14664105`. ### 1. [P1] Write-heavy load paths are not covered `BeMetricsLoader.eventFilter()` only accepts `AFTER_QUERY` events. However, Stream Load emits `STREAM_LOAD_FINISH`, while asynchronous load jobs such as Broker Load emit `LOAD_SUCCEED`. These events are sent directly to `AuditEventProcessor` and are never associated with the per-BE metrics collected by `WorkloadRuntimeStatusMgr`. As a result, typical write-heavy workloads are absent from `query_be_metrics`, which does not satisfy the PR’s stated goal of correlating BE read/write load with query IDs. Simply extending `eventFilter()` is insufficient because these load events currently do not carry the query ID and per-BE metrics map required for correlation. Please add a query-ID-aware metrics path for load completion events, or explicitly narrow the documented feature scope to ordinary SQL queries. ### 2. [P1] Failed Stream Load batches are discarded as successful `BeMetricsStreamLoader.loadBatch()` converts connection and request failures into a `LoadResponse` instead of throwing them. The caller does not validate: - The HTTP response code. - The Stream Load JSON `Status`. - `NumberLoadedRows` and `NumberFilteredRows`. The batch is then unconditionally cleared in the `finally` block. In addition, `max_filter_ratio=1.0` allows an entire batch to be filtered without necessarily failing the request. If the internal table or partition is unavailable, a BE is unhealthy, or Stream Load returns a business-level failure, the whole batch is permanently lost. `discardRowNum` is not updated either. Please define an explicit success contract, retain and retry failed batches with bounded backoff, and accurately account for rows that are eventually discarded. ### 3. [P2] Per-BE objects are allocated even when the feature is disabled `getQueryStatisticsMap()` creates a `BeMetrics` object and a HashMap entry for every `(beId, queryId)` pair during every workload-status cycle. However, `enable_be_metrics_plugin` is disabled by default, and these maps are ignored later when the feature is disabled. This introduces unnecessary allocation and GC overhead proportional to the number of active BE-query pairs under the default configuration. Please preserve the original aggregate-only path while the feature is disabled and construct the per-BE snapshots only when they are actually needed. ### 4. Automated test coverage is required This PR adds internal-table DDL, concurrent queues, batching, Stream Load handling, configuration switches, and new event-routing behavior, but it adds no FE unit tests or regression tests. At minimum, tests should cover: - Aggregated and per-BE metrics across multiple BEs. - Ordinary queries, SQL writes, Stream Load, and asynchronous load jobs. - HTTP failures, Stream Load JSON failures, and fully filtered batches. - Batch retention and retry behavior. - Internal table schema, partitioning, and upgrade-time creation. - No per-BE object construction while the feature is disabled. - Queue-full and shutdown behavior. The current manual verification is insufficient for a kernel feature of this scope. ### Additional review notes - `discardRowNum` is modified from different threads without synchronization. - `close()` neither drains the event queue nor force-flushes the current batch. - The new global variables have no value validation and are read without following the locking requirement documented in `GlobalVariable`. - The PR introduces a queryable internal table and four global variables, so the “Does this need documentation: No” conclusion should be reconsidered. -- 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]
