branch-4.0: [fix](brpc) Make secondary package aliases non-owning #65777#65818
branch-4.0: [fix](brpc) Make secondary package aliases non-owning #65777#65818gavinchou wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review result: request changes
For distinct primary and secondary package names, this change leaves the canonical entries as the sole owners and makes the aliases non-owning, which fixes the direct ClearServices() double deletion. One PR-scoped correctness regression and one missing proof remain:
- If the configured secondary package equals the primary package, the non-owning assignments overwrite the only owning entries, so the service and method statuses leak.
- The Cloud lifecycle test still clears the secondary package specifically to avoid the reported destructor crash, so no runtime test executes this ownership branch.
Critical checkpoint conclusions:
- Goal and behavior: the collision-free shutdown path is improved, but the equal-package configuration introduced in the inline finding violates the ownership contract.
- Scope and clarity: this is a focused branch-4.0 backport of the BRPC patch change; the method and service ownership changes are symmetric and no unrelated source change is included.
- Lifecycle and error handling: distinct-key
ClearServices()is safe under the new ownership flags. The equal-key overwrite is the remaining PR-scoped lifecycle defect. Removal/rollback stale aliases were examined but are unchanged from the base patch and were excluded from the inline set. - Configuration and compatibility: no wire, protobuf, storage, or persistence format changes apply. The unrestricted
secondary_package_namesetting exposes the equal-package edge. - Concurrency, performance, and observability: the changed ownership fields are consumed by deletion guards; no new race, lock, hot-path, or observability regression was substantiated.
- Persistence, transactions, data writes, and FE/BE propagation are not applicable to this one-file third-party ownership delta.
- Validation:
git diff --checkpasses. This runner has nothirdparty/installedtree orprotoc, and the review contract forbids builds, so local validation was static. At the final sweep, GitHub reported Cloud UT and both macOS third-party builds successful; the Linux third-party build was still running, BE UT reported failure, and several regressions were pending. Cloud UT does not exercise the changed branch because the test forces the option empty. - User focus: no additional focus was provided; the full PR was reviewed.
Review completion: the sole changed file and its BRPC/Cloud upstream and downstream paths were reviewed through three rounds. Every candidate was accepted, deduplicated, or dismissed with concrete base-vs-head evidence, and all normal and risk-focused agents returned NO_NEW_VALUABLE_FINDINGS for the final two-comment set on head c5074691e385773ad0206f2737584c92c7d730b6.
| + secondary_method_name.append(md->name()); | ||
| + _method_map[secondary_method_name] = mp; | ||
| + MethodProperty secondary_mp = mp; | ||
| + secondary_mp.own_method_status = false; |
There was a problem hiding this comment.
[P2] Preserve ownership when the alias equals the primary package
secondary_package_name is unrestricted. For MetaService, setting it to the current package doris.cloud makes the derived service and method keys identical to the primary keys. FlatMap::operator[] then overwrites the just-inserted owning entries with these non-owning copies, so RemoveService()/ClearServices() no longer delete the MethodStatus objects or the SERVER_OWNS_SERVICE allocation. Please reject or skip an alias equal to the primary package before assigning the non-owning copies.
| + secondary_method_name.push_back('.'); | ||
| + secondary_method_name.append(md->name()); | ||
| + _method_map[secondary_method_name] = mp; | ||
| + MethodProperty secondary_mp = mp; |
There was a problem hiding this comment.
[P2] Add a runtime regression for this ownership path
cloud/test/meta_server_test.cpp still forces config::secondary_package_name = "" specifically to avoid the brpc::Server destructor crash, so no test executes this changed non-owning branch. A patch-application/build check cannot prove the lifetime behavior. Please run a distinct non-empty alias with an owned service, verify both names resolve, and prove clear/destruction reclaims the shared service/status exactly once; also cover the equal-package ownership case above.
Pick brpc thirdparty patch from #65777 for branch-4.0.\n\nThis PR only changes thirdparty/patches/brpc-1.4.0-secondary-package-name.patch. It intentionally does not include the master-only thirdparty/build-thirdparty.sh change from #65777.\n\nThe fix makes secondary brpc package aliases non-owning to avoid double delete during brpc::Server shutdown.\n\nValidation:\n- git diff --check\n- bash thirdparty/download-thirdparty.sh brpc\n\nIssue: DORIS-27128