Skip to content

branch-4.0: [fix](brpc) Make secondary package aliases non-owning #65777#65818

Open
gavinchou wants to merge 1 commit into
apache:branch-4.0from
gavinchou:gavin-pick-65777-branch-4.0
Open

branch-4.0: [fix](brpc) Make secondary package aliases non-owning #65777#65818
gavinchou wants to merge 1 commit into
apache:branch-4.0from
gavinchou:gavin-pick-65777-branch-4.0

Conversation

@gavinchou

Copy link
Copy Markdown
Contributor

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

@gavinchou
gavinchou requested a review from morningman as a code owner July 20, 2026 09:22
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@gavinchou

Copy link
Copy Markdown
Contributor Author

run buildall

@gavinchou

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_name setting 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 --check passes. This runner has no thirdparty/installed tree or protoc, 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants