branch-4.1: [fix](brpc) Make secondary package aliases non-owning #65777#65819
branch-4.1: [fix](brpc) Make secondary package aliases non-owning #65777#65819gavinchou wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review result: request changes
The collision-free direct brpc::Server destruction path is improved: when primary and secondary names are distinct, the patch leaves one service/status owner and avoids the reported duplicate deletes. Three issues still need resolution:
- Explicit
RemoveServiceand registration rollback leave the secondary aliases in BRPC maps after their shared targets are deleted, enabling stale lookup and restart-time use-after-free. - If
secondary_package_nameequals the primary package, the non-owning assignments overwrite the only owning entries and leak the service/status objects. - The Cloud lifecycle test still disables the exact non-empty alias branch, so lookup and owned teardown remain untested.
Critical checkpoint conclusions:
- Goal and correctness: the normal distinct-key
ClearServicesdouble-free is addressed, but removal/rollback and equal-key behavior keep the lifecycle fix incomplete. - Lifecycle and error handling: M-001 is blocking because public removal and post-insertion failure paths leave dangling map entries.
- Configuration and compatibility: the setting is an unrestricted string and
doris.cloudexactly reaches M-002. No wire, protocol, or storage-format compatibility change applies. - Concurrency: no new race or locking defect was found; BRPC service mutation is READY-state only and direct destruction stops/joins first.
- Scope, performance, and observability: the ownership delta is focused. No new material hot-path or observability regression was substantiated; duplicate alias map walks predate this ownership-only change.
- Persistence, transactions, and data writes: not applicable to this third-party in-memory registration patch.
- Testing and validation: static review only under the runner contract.
git diff --checkis clean, but local third-party sources/install/protoc are absent. Cloud UT is skipped, and the failed macOS-arm64 third-party job stops on an unrelated libunwind patch before reaching BRPC, so there is no runtime proof of the changed branch. - User focus: no additional focus was provided; the complete PR was reviewed.
Review completion: complete. The sole changed file and its BRPC/Doris upstream and downstream paths were reviewed through two rounds; both Round 2 reviewers returned NO_NEW_VALUABLE_FINDINGS, and every candidate was accepted, deduplicated, or dismissed with evidence. The three accepted points are attached inline.
| + _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; |
There was a problem hiding this 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.
| + 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 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.
| + 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 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.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
Pick brpc thirdparty patch from #65777 for branch-4.1.\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