Skip to content

[fix](build) Preserve connector configuration without failing on macOS - #68031

Open
zy-kkk wants to merge 1 commit into
apache:masterfrom
zy-kkk:fix/macos-connector-config-copy
Open

zy-kkk wants to merge 1 commit into
apache:masterfrom
zy-kkk:fix/macos-connector-config-copy

Conversation

@zy-kkk

@zy-kkk zy-kkk commented Sep 15, 2026

Copy link
Copy Markdown
Member

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)

  • Test: Manual tests on macOS and Linux (Iceberg REST fixture container): initialize absent configuration, preserve existing configuration, and propagate copy failure; bash -n and git diff --check; local connector deployment completed.
  • Behavior changed: Yes, existing connector configuration no longer aborts macOS packaging.
  • Does this need documentation: No

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@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?

@zy-kkk

zy-kkk commented Sep 15, 2026

Copy link
Copy Markdown
Member 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.

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 -n returns nonzero when it skips an existing file, while the new if avoids errexit; absent files are seeded and genuine cp errors remain fatal. bash -n and 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.sh seeds with cp -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 --output tree is administrator-managed, and no lock excludes a configuration manager from installing the live .conf after the Bash test. Plain cp then 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.template while 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 if condition is valid under Bash 3.2 and set -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 cp diagnostics 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 cp for 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.

Comment thread build.sh Outdated
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}"

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

zy-kkk added a commit to zy-kkk/doris that referenced this pull request Sep 15, 2026
### 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
zy-kkk added a commit to zy-kkk/doris that referenced this pull request Sep 15, 2026
### 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
@zy-kkk
zy-kkk force-pushed the fix/macos-connector-config-copy branch from 14a107f to d41c6d1 Compare September 15, 2026 16:50
@zy-kkk

zy-kkk commented Sep 15, 2026

Copy link
Copy Markdown
Member 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 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.sh passes. 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 -n is 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 cp diagnostics 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 cp for 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.

@zy-kkk

zy-kkk commented Sep 15, 2026

Copy link
Copy Markdown
Member Author

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
@zy-kkk
zy-kkk force-pushed the fix/macos-connector-config-copy branch from d41c6d1 to f7d9b75 Compare September 16, 2026 15:06
@zy-kkk

zy-kkk commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 16802 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f7d9b759abcbda4aa431513e43476a1a8bf45758, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17568	3080	3066	3066
q2	2081	254	227	227
q3	10244	898	499	499
q4	4666	252	198	198
q5	7678	567	382	382
q6	138	117	93	93
q7	534	498	381	381
q8	9241	892	912	892
q9	3550	2363	2367	2363
q10	6506	858	702	702
q11	391	197	200	197
q12	609	273	209	209
q13	18108	1526	1136	1136
q14	157	159	142	142
q15	q16	435	398	371	371
q17	1321	895	812	812
q18	3134	2297	2232	2232
q19	1277	917	796	796
q20	446	294	207	207
q21	5652	1671	1927	1671
q22	339	265	226	226
Total cold run time: 94075 ms
Total hot run time: 16802 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	3441	3375	3351	3351
q2	494	388	368	368
q3	2279	2322	2167	2167
q4	1208	1167	896	896
q5	2184	2122	2101	2101
q6	168	122	85	85
q7	1008	921	854	854
q8	1594	1393	1396	1393
q9	3147	3128	3127	3127
q10	1874	1809	1652	1652
q11	366	269	250	250
q12	454	440	357	357
q13	1491	1550	1163	1163
q14	174	165	172	165
q15	q16	394	400	376	376
q17	3598	3211	3203	3203
q18	4808	4438	4775	4438
q19	850	909	883	883
q20	1003	989	828	828
q21	3869	3260	3229	3229
q22	399	348	316	316
Total cold run time: 34803 ms
Total hot run time: 31202 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 82472 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit f7d9b759abcbda4aa431513e43476a1a8bf45758, data reload: false

query5	4245	420	327	327
query6	385	137	123	123
query7	4954	414	229	229
query8	290	130	122	122
query9	8687	2909	2909	2909
query10	389	226	186	186
query11	5371	1041	916	916
query12	126	78	76	76
query13	1187	438	316	316
query14	6037	2224	2112	2112
query14_1	1995	1977	1986	1977
query15	176	119	114	114
query16	922	381	362	362
query17	796	460	355	355
query18	2345	332	245	245
query19	170	139	111	111
query20	73	70	75	70
query21	198	102	88	88
query22	5578	5433	5552	5433
query23	6741	6341	6237	6237
query23_1	6250	6135	6032	6032
query24	7260	1097	790	790
query24_1	766	765	810	765
query25	426	299	252	252
query26	1240	222	133	133
query27	2783	416	259	259
query28	4670	1527	1502	1502
query29	933	441	337	337
query30	242	153	129	129
query31	819	405	337	337
query32	125	72	73	72
query33	489	223	178	178
query34	1019	838	492	492
query35	408	413	354	354
query36	585	558	515	515
query37	125	84	74	74
query38	1007	844	841	841
query39	499	473	462	462
query39_1	456	426	437	426
query40	203	92	83	83
query41	59	58	56	56
query42	79	76	72	72
query43	241	242	211	211
query44	992	542	562	542
query45	111	103	106	103
query46	776	860	547	547
query47	770	749	712	712
query48	304	308	246	246
query49	539	243	198	198
query50	728	265	196	196
query51	8309	8202	8102	8102
query52	72	68	63	63
query53	195	194	147	147
query54	216	170	204	170
query55	71	62	57	57
query56	179	168	162	162
query57	692	658	659	658
query58	189	192	158	158
query59	1205	1232	1089	1089
query60	243	190	174	174
query61	109	106	100	100
query62	361	208	178	178
query63	172	141	138	138
query64	2730	680	611	611
query65	1667	1593	1590	1590
query66	1869	256	218	218
query67	9739	9963	9617	9617
query68	2886	1199	738	738
query69	338	216	196	196
query70	677	631	651	631
query71	261	176	166	166
query72	2233	1651	1433	1433
query73	652	586	333	333
query74	1845	1230	1140	1140
query75	1177	1106	962	962
query76	2283	705	537	537
query77	241	262	214	214
query78	4012	3724	3339	3339
query79	2418	815	587	587
query80	1600	311	278	278
query81	491	158	135	135
query82	612	130	95	95
query83	276	207	189	189
query84	293	115	87	87
query85	772	336	309	309
query86	389	175	166	166
query87	1035	980	916	916
query88	2768	2122	2112	2112
query89	289	195	172	172
query90	1977	125	130	125
query91	127	119	93	93
query92	70	64	61	61
query93	1562	1112	711	711
query94	627	253	184	184
query95	512	315	225	225
query96	821	602	263	263
query97	1033	1045	1004	1004
query98	158	137	142	137
query99	412	346	308	308
Total cold run time: 177893 ms
Total hot run time: 82472 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 14.63 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit f7d9b759abcbda4aa431513e43476a1a8bf45758, data reload: false

query1	0.00	0.00	0.01
query2	0.09	0.03	0.03
query3	0.25	0.11	0.11
query4	1.61	0.09	0.09
query5	0.18	0.16	0.17
query6	1.24	0.70	0.68
query7	0.04	0.01	0.00
query8	0.04	0.02	0.03
query9	0.28	0.22	0.21
query10	0.35	0.35	0.34
query11	0.16	0.11	0.12
query12	0.15	0.12	0.12
query13	0.31	0.30	0.30
query14	0.44	0.45	0.44
query15	0.36	0.35	0.35
query16	0.22	0.22	0.23
query17	0.64	0.69	0.67
query18	0.16	0.17	0.17
query19	1.12	1.05	1.16
query20	0.01	0.01	0.02
query21	15.46	0.16	0.11
query22	5.07	0.04	0.05
query23	16.15	0.25	0.11
query24	2.98	0.35	0.24
query25	0.11	0.04	0.04
query26	0.75	0.17	0.11
query27	0.04	0.03	0.04
query28	3.65	0.61	0.29
query29	12.43	3.17	2.56
query30	0.26	0.12	0.13
query31	2.75	0.38	0.18
query32	3.51	0.33	0.23
query33	1.34	1.52	1.48
query34	15.37	2.23	1.79
query35	1.78	1.72	1.75
query36	0.47	0.29	0.29
query37	0.07	0.04	0.03
query38	0.04	0.04	0.03
query39	0.03	0.02	0.02
query40	0.12	0.08	0.07
query41	0.08	0.02	0.02
query42	0.03	0.02	0.02
query43	0.03	0.03	0.02
Total cold run time: 90.17 s
Total hot run time: 14.63 s

@zy-kkk

zy-kkk commented Sep 17, 2026

Copy link
Copy Markdown
Member 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.

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 retained cp -n does not close the write-boundary race.
  • Lifecycle: unzip -o refreshes the plugin-owned template, jar cleanup leaves the live config alone, and FE reads <name>.conf once 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.sh and git diff --check passed, 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.

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