HappenLee commented on PR #67762: URL: https://github.com/apache/doris/pull/67762#issuecomment-5695522698
/review Please review the full updated PR at 9b58d882615cf8922e57804e3a275cc168800664, especially the two latest P1 findings from review 5220502872. RPC preparation and internal read workers now run on a dedicated BE-wide `FifoThreadPool` with `CpuInfo::num_cores()` pthreads. Both use nonblocking admission; parent preparation returns after dispatch, including on a one-thread pool. This removes this RPC's dependence on the TaskExecutor admission/bookkeeping paths implicated in those findings, without modifying the shared scheduler. Validation: ASAN test build and all 36 related BE unit tests passed, including real-pool saturation with an active read, shutdown rejection, queued-task destruction, single-thread progress, concurrency bounds, and completion/request lifetime. SQL regression, storage integration, and performance benchmarks have not been run. Please also check remaining request-wiring and integration-test coverage gaps. <details> <summary>Self-review checkpoints</summary> | Checkpoint | Conclusion | | --- | --- | | Goal | A shared dedicated pool has one worker per CPU core; tests prove asynchronous progress and bounded concurrency. | | Scope | Only rowid dispatch, its RPC service, configuration wording, and corresponding tests change. No shared TaskExecutor or bthread helper changes. | | Concurrency | Workers atomically claim disjoint tasks; statistics retain their existing mutex. No parent waits for work in the same pool. | | Lifetime | Request state is retained until accepted readers exit. Completion clears closures; discarded queued closures cancel exactly once. Active pool threads are joined before service I/O teardown. | | Configuration | No additional setting; the existing mutable per-request limit is snapshotted. Pool size is fixed at service startup. | | Compatibility | No RPC/Thrift/storage format change in this revision. Default reads stay serial, with preparation moved to the dedicated pool. | | Parallel paths | Shared-nothing and cloud use the common service. External file reads retain their scanner path; legacy multiget is unchanged. | | Error branches | Full/stopped admission returns SERVICE_UNAVAILABLE. Pre-publication exceptions become errors; partial submissions retain accepted workers. | | Coverage | 36 ASAN tests passed, including real-pool failure and lifetime scenarios; no claim of end-to-end SQL or performance validation. | | Test results | GTest XML was generated by run-be-ut.sh: 36 tests, zero failures/errors. No handwritten regression output. | | Observability | Existing latency/cache profiles remain. Pool startup logs its size; rejection errors include queue/thread/shutdown state. | | Persistence | No transaction, EditLog, or persistent-format changes. | | Writes | Only read-response construction changes; failed requests do not return success. | | FE/BE wiring | Existing session/thrift/protobuf plumbing remains; the BE request test verifies dispatch to the supplied pool. No new FE-to-protobuf integration test. | | Performance | One bounded shared pool and bounded workers per request; batching, deduplication, and scatter order remain. No benchmark results yet. | | Other concerns | Workload Groups share this pool; WG scheduling isolation is deferred. Headers use forward declarations. Formatting, build hygiene, and changed-line static analysis were checked. | </details> -- 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]
