github-actions[bot] commented on code in PR #65819:
URL: https://github.com/apache/doris/pull/65819#discussion_r3613795413
##########
thirdparty/patches/brpc-1.4.0-secondary-package-name.patch:
##########
@@ -27,17 +27,21 @@ index 2087cbcf..7aede561 100644
+ secondary_method_name.append(secondary_full_name);
+ secondary_method_name.push_back('.');
+ secondary_method_name.append(md->name());
-+ _method_map[secondary_method_name] = mp;
++ MethodProperty secondary_mp = mp;
++ secondary_mp.own_method_status = false;
Review Comment:
[P2] Preserve ownership when the alias equals the primary package
`secondary_package_name` is unrestricted; for MetaService, setting it to its
current package `doris.cloud` makes `secondary_full_name` equal
`sd->full_name()` and the derived method keys equal `md->full_name()`.
`FlatMap::operator[]` then overwrites the just-inserted owning entries with
these non-owning copies, so `ClearServices`/`RemoveService` no longer deletes
the `MethodStatus` or the `SERVER_OWNS_SERVICE` allocation. Reject or skip an
alias equal to the primary name before assigning it, and preflight any other
occupied alias key.
##########
thirdparty/patches/brpc-1.4.0-secondary-package-name.patch:
##########
@@ -27,17 +27,21 @@ index 2087cbcf..7aede561 100644
+ secondary_method_name.append(secondary_full_name);
+ secondary_method_name.push_back('.');
+ secondary_method_name.append(md->name());
-+ _method_map[secondary_method_name] = mp;
++ MethodProperty secondary_mp = mp;
++ secondary_mp.own_method_status = false;
++ _method_map[secondary_method_name] = secondary_mp;
+ }
if (is_idl_support && sd->name() != sd->full_name()/*has ns*/) {
MethodProperty mp2 = mp;
mp2.own_method_status = false;
-@@ -1306,6 +1322,9 @@ int
Server::AddServiceInternal(google::protobuf::Service* service,
+@@ -1306,6 +1324,11 @@ int
Server::AddServiceInternal(google::protobuf::Service* service,
is_builtin_service, svc_opt.ownership, service, NULL };
_fullname_service_map[sd->full_name()] = ss;
_service_map[sd->name()] = ss;
+ if (!secondary_full_name.empty()) {
-+ _fullname_service_map[secondary_full_name] = ss;
++ ServiceProperty secondary_ss = ss;
++ secondary_ss.ownership = SERVER_DOESNT_OWN_SERVICE;
++ _fullname_service_map[secondary_full_name] = secondary_ss;
Review Comment:
[P1] Erase these aliases when removing the service
Making the alias non-owning avoids the destructor double free, but BRPC
1.4.0 `RemoveMethodsOf` erases only `md->full_name()` (plus the IDL
no-namespace alias), and `RemoveService` erases only
`sd->full_name()`/`sd->name()`. Therefore `AddService` with a secondary package
followed by `RemoveService`—or an `AddServiceInternal` rollback—deletes the
primary `MethodStatus` and, on owning paths, the service while these secondary
entries remain. A later `Find*` returns dangling state, and `Start()`
dereferences the freed status/service. Please erase the secondary method and
service keys on every removal/rollback before deleting the shared objects.
##########
thirdparty/patches/brpc-1.4.0-secondary-package-name.patch:
##########
@@ -27,17 +27,21 @@ index 2087cbcf..7aede561 100644
+ secondary_method_name.append(secondary_full_name);
+ secondary_method_name.push_back('.');
+ secondary_method_name.append(md->name());
-+ _method_map[secondary_method_name] = mp;
++ MethodProperty secondary_mp = mp;
Review Comment:
[P2] Add a runtime regression for this ownership branch
The Cloud lifecycle test currently forces `config::secondary_package_name =
""` specifically to avoid `brpc::Server::~Server()` crashing, so no test
executes this changed non-owning branch. The patch/download checks prove only
that the third-party patch applies. Please enable a distinct non-empty alias in
a test, assert both service/method names resolve, and destroy or
`ClearServices()` an owning server; the removal and equal-package cases above
should also be covered.
--
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]
