[fix](brpc) Make secondary package aliases non-owning#65777
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
71712a5 to
6d54960
Compare
gavinchou
changed the title
|
run buildall |
|
Updated this PR based on the DORIS-27128 TLS/cloud context. The previous variant regression retry change was removed; the PR now fixes OSS BE/cloud starter handling for protocols excluded by tls_excluded_protocols. |
|
/review |
6d54960 to
f3ba3c2
Compare
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
gavinchou
changed the title
|
Corrected again after reviewing the secondary_package shutdown path. The TLS starter change has been removed. This PR now fixes the brpc secondary_package_name patch by making secondary service/method aliases non-owning, avoiding double delete during brpc::Server::ClearServices(). |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
![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 normal destructor path is improved: when primary and secondary names are distinct, the patch leaves one owner and avoids the duplicate deletes. Three issues still need resolution:
- Explicit
RemoveServiceand registration rollback leave the new aliases in BRPC maps after shared targets are deleted, enabling stale lookup andStart()use-after-free. - If
secondary_package_nameequals the primary package,FlatMapassignment overwrites the only owning entries with non-owning copies. - The Cloud lifecycle test still disables the exact non-empty alias branch, so lookup and owned teardown are untested.
Critical checkpoint conclusions:
- Goal and tests: the direct
ClearServicesdouble-free is addressed, but runtime regression coverage and removal/error paths are incomplete. - Scope and clarity: this is one focused patch; method and service ownership changes are symmetrical.
- Lifecycle and error handling: M-001 is blocking; cleanup must cover explicit removal and every rollback.
- Concurrency: no new race or lock issue was found; BRPC mutation is
READY-only and destruction stops/joins beforeClearServices. - Configuration and compatibility: no protocol or storage-format change applies; M-002 is the uncovered configuration edge. No persistence, transaction, FE/BE propagation, data-write, observability, or material performance issue applies.
- Testing: static review only under the runner contract;
thirdparty/installedandprotocare absent. Cloud UT does not cover the changed branch because the test clears the configuration. - User focus: no additional focus was provided; the full PR was reviewed.
Review completion: the single changed file and its BRPC/Cloud upstream and downstream paths were reviewed through three rounds; 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 removal paths, the service while these secondary map entries remain. A later Find* returns dangling state, and Start() dereferences the freed status/service. Please track and 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 alias-key collision.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
f3ba3c2 to
d9f51d1
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes.
I found two additional build-toolchain issues on this head. The existing BRPC lifecycle threads also remain unresolved: explicit removal and rollback leave aliases behind, alias-key collisions can replace owning records, and no runtime test exercises a non-empty alias through cleanup.
Critical checkpoint conclusions:
- Goal and proof: the BRPC ownership flags fix only the collision-free direct-clear double deletion; the broader lifecycle goal remains incomplete and has no runtime proof. The default rustup path selects Cargo 1.91, but explicit-tool and actual-compiler paths do not reliably pin the intended Rust/Cargo pair.
- Scope and clarity: the ownership delta is small; the Rust workaround has broader environment effects and must preserve explicit overrides while validating the compiler that triggers the failure.
- Concurrency and lifecycle: no new lock, thread, or data-race defect was found. Registration and removal remain READY-state operations, but the deterministic alias removal/collision defects remain in the existing threads.
- Configuration, compatibility, and parallel paths: the existing secondary-package setting is startup-sampled, successful full-name alias dispatch is compatible, and no wire/storage format changes occur. Removal, rollback, equal-key, custom/offline toolchain, compiler-override, Linux, and macOS paths were checked; the two inline findings cover the remaining patch-scoped gaps.
- Error handling, performance, and observability:
set -emakes the premature rustup download fatal before the selected custom Cargo is checked. No additional Status/exception, memory, hot-path, or observability regression was substantiated. - Persistence, data writes, transactions, and FE/BE transport are not applicable to these changes.
- Tests and results: existing syntax and patch-application checks do not cover runtime aliases or custom/offline/compiler-mismatch toolchains. This review was static-only as required; the Linux and both macOS third-party builds, plus several broader checks, were still pending at submission time.
- User focus: no additional focus was supplied.
Overall conclusion: REQUEST_CHANGES.
| if rustup toolchain list | grep -q '^1.91.0'; then | ||
| cargo_env+=("RUSTUP_TOOLCHAIN=1.91.0") | ||
| if ! rustup toolchain list | grep -Eq '^1\.91\.0([[:space:]-]|$)'; then | ||
| rustup toolchain install "${required_rust_version}" --profile minimal |
There was a problem hiding this comment.
[P2] Respect an explicitly selected Cargo before invoking rustup
cargo_bin may come from LANCE_C_CARGO/CARGO, but this branch runs whenever any unrelated rustup is on PATH. A network-restricted builder with an exact /opt/rust-1.91/bin/cargo (and matching RUSTC) plus rustup containing only stable now attempts rustup toolchain install 1.91.0 and exits under set -e before checking the valid custom toolchain; LANCE_C_CARGO_OFFLINE=ON is only applied later. Please auto-manage rustup only for the default/rustup Cargo path and validate explicit tool overrides directly.
| exit 0; | ||
| }'; then | ||
| echo "lance-c requires Rust/Cargo ${required_rust_version} or newer, but found ${cargo_version}." | ||
| if [[ "${cargo_version}" != "${required_rust_version}" ]]; then |
There was a problem hiding this comment.
[P2] Pin the compiler that triggers this failure
The reported ethnum failure is produced by rustc, but this gate validates only cargo --version. Cargo 1.91.0 can still execute a newer compiler through inherited RUSTC/CARGO_BUILD_RUSTC (or a standalone Cargo paired with a newer rustc on PATH), so that setup passes here and reaches the same E0512 path. Please select and version-check the actual compiler Cargo will use, then pass the paired 1.91.0 toolchain consistently into the build.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
Proposed changes
Fix DORIS-27128 in the brpc secondary package alias implementation.
The existing brpc secondary_package_name patch registers the secondary service name by copying the primary ServiceProperty, and registers secondary methods by copying the primary MethodProperty as owning entries. During brpc::Server destruction, ClearServices() iterates all service and method map entries:
This matches the cloud MetaService secondary package shutdown/coredump path. The patch now makes secondary aliases non-owning:
The previous variant regression retry change and the TLS starter change have both been removed from this PR.
The failed thirdparty CI was unrelated to the brpc patch application: brpc had already built and installed successfully, then Linux/macOS failed while building lance-c because runner cargo selected a newer Rust toolchain and ethnum v1.5.2 failed with E0512. The PR now pins lance-c to Rust/Cargo 1.91.0 through rustup when available, and fails early with a clear message otherwise.
Testing
Issue: DORIS-27128