Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Requesting changes for one P2 correctness issue: the new check-then-copy sequence can overwrite an administrator configuration installed during the gap between the check and cp.
Critical checkpoint conclusions
- Goal and proof: The stable-state goal is sound: Apple
cp -nreturns nonzero when it skips an existing file, while the newifavoidserrexit; absent files are seeded and genuinecperrors remain fatal.bash -nand isolated seed/preserve/failure probes passed. The accepted race means the never-overwrite goal is not yet fully achieved. - Scope and clarity: The source change is small and generic, but removing no-clobber at the actual write boundary is the wrong tradeoff. Non-blocking cleanup: the six connector assembly comments still say
build.shseeds withcp -n; update them to describe the semantic seed-once contract. - Concurrency: Packaging itself is sequential and has no in-process lock ordering concern. However, a persistent/custom
--outputtree is administrator-managed, and no lock excludes a configuration manager from installing the live.confafter the Bash test. Plaincpthen truncates that file; a deterministic interleaving reproduces the loss. Same-output concurrent builds are already unsafe more broadly and are not a separate finding. - Lifecycle: Connector ZIP extraction refreshes jars and
.conf.templatewhile deliberately retaining the live.conf; FE reads that config at plugin startup. The changed write boundary violates this preservation lifecycle only in M1; stable first-deploy and rebuild paths otherwise behave correctly. - Configuration and compatibility: No Doris config item, dynamic-reload contract, protocol, storage format, function symbol, persisted state, or rolling-upgrade wire compatibility changes. Existing live configs remain compatible outside the race.
- Parallel paths: The quoted generic loop covers all six current ZIP-root templates (
jdbc,trino-connector,hms,paimon,iceberg,adbc). Filesystem and authorization packaging have no parallel live-config seeding path requiring the same change. - Condition and error handling: The
ifcondition is valid under Bash 3.2 andset -e; missing globs remain guarded, and real copy failures remain observable and fatal. No other silent-error path was found. - Tests/results: No test or result file changes are present, and no repository test covers connector output-tree seeding. The author reports manual macOS/Linux checks, but those builds were not independently run under this review's no-build contract. Add a deterministic check-then-install fixture for the accepted race alongside absent/existing/error cases.
- Observability: Existing
cpdiagnostics and fatal status are sufficient; no metric or additional runtime logging is warranted. - Transactions, persistence, data writes, FE/BE variables, memory, and static initialization: Not involved.
- Performance: One shell builtin per template is negligible and avoids spawning
cpfor stable existing files; no other performance issue was found. - Other issues / user focus: No additional user focus was supplied, and the full changed-file/lifecycle sweep found no distinct issue beyond the inline P2.
Review status: complete static review at head 775db43e060967b0dad6dce26e0ba2d531ac3c13; one inline change request.
| cp -n "${conn_conf_tpl}" "${conn_conf_tpl%.template}" | ||
| # macOS cp -n returns 1 when the destination exists, which aborts this script under set -e. | ||
| if [[ ! -e "${conn_conf_tpl%.template}" ]]; then | ||
| cp "${conn_conf_tpl}" "${conn_conf_tpl%.template}" |
There was a problem hiding this comment.
[P2] Keep no-clobber protection at the write boundary
This pre-check is not atomic with the following copy. If build.sh observes no live config and a configuration manager installs one before cp opens the destination, plain cp truncates that newly installed administrator file, contradicting the block's never overwritten contract. A deterministic check -> install admin -> copy sequence preserves admin with the old cp -n but replaces it with the template here. Please retain no-clobber behavior inside the guard (a raced-in file may safely stop the build) or use an atomic create-if-absent helper, and cover this interleaving in the fixture.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68031 Problem Summary: A live connector configuration installed after the existence check could be overwritten by plain cp. Keep the check to skip existing configurations on macOS, and retain cp -n at the copy boundary to preserve a configuration installed in that gap. Describe the seed-only-if-absent contract in the six connector assembly comments. ### Release note Preserve connector configurations installed during build packaging. ### Check List (For Author) - Test: Manual fixture on macOS Bash 3.2 and Linux (Iceberg REST fixture container): absent, existing, empty glob, real copy failure, and deterministic check-then-install race. The race test fails against the previous version. bash -n, assembly XML parsing, and git diff --check passed. - Behavior changed: Yes. A raced-in configuration is preserved; macOS may stop packaging in this case, while normal repeated packaging continues. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68031 Problem Summary: On macOS, cp -n returns 1 when an existing connector configuration is skipped. Under set -e, this aborts plugin deployment and skips subsequent packaging steps. Check whether the live configuration exists before copying, while keeping cp -n to preserve a configuration installed between the check and copy. Real copy failures remain fatal; a raced-in configuration may also stop packaging on macOS. ### Release note Fix incremental build packaging stopping on macOS when connector configuration already exists. ### Check List (For Author) - Test: Manual fixture on macOS Bash 3.2 and Linux (Iceberg REST fixture container): initialization, existing configuration preservation, empty template glob, real copy failure, and deterministic check-then-install race. The race fixture fails with the prior plain-cp implementation. bash -n and git diff --check passed. - Behavior changed: Yes. Existing configuration no longer aborts normal macOS packaging, and no-clobber protection is retained when copying. - Does this need documentation: No
14a107f to
d41c6d1
Compare
|
/review |
There was a problem hiding this comment.
Automated code review summary for head d41c6d179877f8f243217ebbf24318f630be528d.
No new valuable findings. The previously reported P2 no-clobber race is addressed at the current head: the existence guard avoids the ordinary macOS cp -n nonzero skip under set -e, while retaining cp -n at the write boundary protects a live configuration installed after the precheck. Genuine copy errors remain fatal.
Critical checkpoint conclusions:
- Goal and proof: The change achieves the stated shell-portability goal by statically tracing absent, existing, and raced-in destinations.
bash -n build.shpasses. No tracked automated fixture is added; the PR reports manual macOS/Linux coverage including initialization, preservation, empty glob, real failure, and the deterministic race, which was not independently rerun here. - Scope and clarity: The five-line change is focused, generic across connector templates, and locally documents both checks.
- Concurrency: Packaging is sequential, but an external configuration manager can race the seed. The retained
cp -nis the no-clobber boundary; a BSD/macOS raced-in skip may safely abort, while GNU may continue, and neither overwrites the administrator file. There are no in-process locks or lock-order concerns. - Lifecycle: ZIP extraction refreshes jars and templates while preserving each live
.conf; the revised seed-once path maintains that upgrade lifecycle. No static/global initialization or ownership lifecycle is involved. - Configuration and compatibility: No Doris configuration item, dynamic-reload contract, protocol, symbol, storage format, persisted state, or rolling-upgrade wire behavior changes. Existing live connector settings remain preserved.
- Parallel paths and conditions: The generic connector loop covers all current ZIP-root templates; filesystem and authorization packaging have no parallel live-config seeding path. The unmatched-glob guard, per-template iteration, and real-error propagation remain correct.
- Tests and results: There are no test/result-file changes or repository test for this shell path. Author-reported manual negative cases are proportionate to the focused fix; only the independent Bash syntax check was run under this no-build review contract.
- Observability: Existing
cpdiagnostics and fatal exit behavior are sufficient; no runtime metric or log is warranted. - Transactions, persistence, database data writes, FE/BE variable passing, memory safety, and static initialization: Not involved. The only write is packaging-time seeding of a live config, whose no-overwrite invariant is preserved.
- Performance: One shell existence test per template is negligible and avoids spawning
cpfor stable existing files. - Other issues and user focus: No additional user focus was supplied, and the final changed-file and unresolved-candidate sweep found no distinct issue.
Opinion: approve. Review status: complete static review after one converged normal/risk-focused round; no new inline comments.
|
run buildall |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68031 Problem Summary: On macOS, cp -n returns 1 when an existing connector configuration is skipped. Under set -e, this aborts plugin deployment and skips subsequent packaging steps. Check whether the live configuration exists before copying, while keeping cp -n to preserve a configuration installed between the check and copy. Real copy failures remain fatal; a raced-in configuration may also stop packaging on macOS. ### Release note Fix incremental build packaging stopping on macOS when connector configuration already exists. ### Check List (For Author) - Test: Manual fixture on macOS Bash 3.2 and Linux (Iceberg REST fixture container): initialization, existing configuration preservation, empty template glob, real copy failure, and deterministic check-then-install race. The race fixture fails with the prior plain-cp implementation. bash -n and git diff --check passed. - Behavior changed: Yes. Existing configuration no longer aborts normal macOS packaging, and no-clobber protection is retained when copying. - Does this need documentation: No
d41c6d1 to
f7d9b75
Compare
|
run buildall |
TPC-H: Total hot run time: 16802 ms |
TPC-DS: Total hot run time: 82472 ms |
ClickBench: Total hot run time: 14.63 s |
|
/review |
There was a problem hiding this comment.
Review result: changes requested — the existing P2 remains unresolved
The steady-state macOS behavior is improved: a regular pre-existing connector config is skipped before cp -n, an absent config is seeded, and a genuine copy failure in the then body remains fatal under set -e. However, the existing no-clobber thread still applies.
The cp -n retained at the write boundary is not an atomic create-if-absent primitive on macOS. Apple's pinned cp first snapshots destination absence with fstatat, then later opens the dne path with O_CREAT | O_TRUNC and no O_EXCL. A configuration manager can therefore install a file or symlink after cp performs its internal check but before that open, and the build can still truncate administrator data. This is the same location, invariant, interleaving, and outcome already covered by the existing P2, so I am not adding a duplicate inline comment. Please keep that thread open and use a portable atomic create-if-absent publication step, with a deterministic interleaving fixture around the actual primitive.
Critical checkpoint conclusions:
- Goal and proof: the ordinary pre-existing-config macOS abort is fixed, but the full stated “never overwritten” lifecycle contract is not yet met under the existing race. The PR reports manual macOS/Linux coverage; this one-file diff adds no automated fixture for the publication interleaving.
- Scope and clarity: the four-line shell change is otherwise small, focused, quoted correctly, and preserves the generic template glob and destination mapping.
- Concurrency: connector packaging is sequential, but an external config installer or concurrent packager can race on the live
.conf. There is no lock, and Apple's final open is not exclusive, so the retainedcp -ndoes not close the write-boundary race. - Lifecycle:
unzip -orefreshes the plugin-owned template, jar cleanup leaves the live config alone, and FE reads<name>.confonce at plugin startup with documented fallback behavior. No static-initialization issue or ownership cycle is involved. - Configuration and compatibility: no Doris config item, dynamic-reload contract, FE/BE variable, protocol, persisted format, function symbol, or rolling-upgrade compatibility surface changes.
- Parallel and conditional paths: no second connector-config seeding path needs the same edit. The new condition is locally documented and handles the stable existing/absent cases; wrong-type and dangling-symlink states do not establish a distinct regression.
- Error handling and observability: ordinary copy errors remain nonzero and visible. Existing FE config-load logging and build failure output are sufficient; no new metric is warranted.
- Transactions, data writes, persistence, memory, and locks: not applicable to this packaging-only change.
- Performance: one extra filesystem existence check per template is negligible relative to unzip and packaging work.
- Tests/results: static-only review per the runner contract.
bash -n build.shandgit diff --checkpassed, and an isolated Linux branch harness confirmed stable existing, absent, valid-symlink, and injected-failure behavior. No repository build was run and no generated result file changed. - User focus: no additional focus points were supplied.
The complete diff, connector template/loader lifecycle, existing comments, normal full review, and risk-focused review were covered. Both Round 1 reviewers returned no new valuable findings distinct from the existing thread; all candidates were adjudicated, and the review converged in Round 1.
What problem does this PR solve?
Issue Number: N/A
Problem Summary: On macOS, cp -n returns 1 when an existing connector configuration is skipped. Under set -e, rebuilding stops during plugin deployment and leaves subsequent connectors stale. Copy the template only when the live configuration does not exist, preserving existing settings and propagating real copy failures on macOS and Linux.
Release note
Fix incremental build packaging stopping on macOS when connector configuration already exists.
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)