wwbmmm commented on PR #3712: URL: https://github.com/apache/brpc/pull/3712#issuecomment-6704521622
1. This PR is not reviewable as a QoS change. Its ~4.5k added lines also carry the runtime-recovery work from #3708 plus changes unrelated to QoS: `--urma_tp_type` path selection, `--urma_copy_send`, the recovery policy, the send-failure injection harness, and a new `.github/workflows/ci-urma.yml`. Please merge #3708 first and rebase so this PR contains only the QoS delta, and keep the CI workflow in the PR that introduces the recovery support. Otherwise reviewers cannot separate what QoS needs from what the dependency already provides. 2. Adding `UrmaQosOptions` to `ChannelOptions`, `ServerOptions`, `SocketOptions` and `Acceptor` makes this a public API change on the core headers (`channel.h`/`server.h`/`socket.h` now include `brpc/urma/urma_qos.h`). The requirement is reasonable, but the PR should justify why a process-level default (the current behavior, which can stay a flag) is insufficient and how many distinct settings a process can carry: each distinct (priority, SL) combination needs its own Jetty per device, and prepared-Jetty reuse is already made conditional on a priority match, so every business channel with a custom priority adds Jetty creation/teardown cost. Please document the expected bound and the behavior when a device exposes few priorities. -- 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]
