Copilot commented on code in PR #63898:
URL: https://github.com/apache/doris/pull/63898#discussion_r3323553965
##########
be/src/runtime/cdc_client_mgr.cpp:
##########
@@ -149,10 +152,26 @@ Status
CdcClientMgr::start_cdc_client(PRequestCdcClientResult* result) {
_child_pid.store(0);
}
#endif
- } else {
+ } else if (!_adopted_external.load()) {
LOG(INFO) << "CDC client has never been started";
}
+#ifndef BE_TEST
+ // Adopt an externally-managed cdc_client if the port already answers
+ // healthy (e.g. one started manually for debug / hotfix).
+ {
+ std::string adopt_response;
+ if (check_cdc_client_health(1, 0, adopt_response).ok()) {
+ if (!_adopted_external.exchange(true)) {
+ LOG(INFO) << "Adopting external cdc client on port "
+ << doris::config::cdc_client_port;
+ }
+ return Status::OK();
Review Comment:
The adoption probe accepts any localhost service whose `/actuator/health`
response contains `UP` as a CDC client. That changes the previous failure mode
for a port collision into sending CDC requests to whatever process owns the
port, so a non-CDC or untrusted local service can be adopted accidentally.
Please validate a CDC-specific/authenticated endpoint (for example one that
proves it has the expected cluster token) before returning success, or gate
external adoption behind an explicit opt-in.
##########
be/src/runtime/cdc_client_mgr.cpp:
##########
@@ -149,10 +152,26 @@ Status
CdcClientMgr::start_cdc_client(PRequestCdcClientResult* result) {
_child_pid.store(0);
}
#endif
- } else {
+ } else if (!_adopted_external.load()) {
LOG(INFO) << "CDC client has never been started";
}
+#ifndef BE_TEST
+ // Adopt an externally-managed cdc_client if the port already answers
+ // healthy (e.g. one started manually for debug / hotfix).
+ {
+ std::string adopt_response;
+ if (check_cdc_client_health(1, 0, adopt_response).ok()) {
+ if (!_adopted_external.exchange(true)) {
+ LOG(INFO) << "Adopting external cdc client on port "
+ << doris::config::cdc_client_port;
+ }
+ return Status::OK();
Review Comment:
The main production behavior added by this PR is not covered by the new
tests: under `BE_TEST`, this adoption probe is compiled out, so the tests only
exercise the `_adopted_external` accessor state rather than the pre-fork
adoption/fallback path. Please add a test seam or integration-style test that
exercises the health-probe adoption path and the failed-probe fork fallback so
regressions in this lifecycle logic are caught.
##########
be/src/common/config.cpp:
##########
@@ -73,6 +73,8 @@ DEFINE_Int32(arrow_flight_sql_port, "8050");
DEFINE_Int32(cdc_client_port, "9096");
+DEFINE_mString(cdc_client_java_opts, "");
Review Comment:
The PR description says the shipped BE config defaults
`cdc_client_java_opts` to `-XX:+ExitOnOutOfMemoryError`, but this config is
registered with an empty default and `conf/be.conf` does not add the advertised
entry. Please either set/document the advertised default in the shipped config
or update the PR/design to make clear that the OOM flag is hard-coded
separately and this option only supplies additional JVM arguments.
--
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]
