Repository navigation
Conversation
### What problem does this PR solve? Routine Load forwards custom Kafka properties to librdkafka without limiting the configuration surface. Restrict the exposed options before FE metadata requests and BE client configuration. - Share explicit property allowlists through Thrift constants, with FE validation for CREATE/ALTER and persisted jobs, and BE validation before any custom `Conf::set()` call. - Reject command/module/provider options and unknown properties. Certificate paths must reference Doris-managed `FILE:` objects. - Disable librdkafka's internal Kerberos ticket acquisition and renewal. Only a zero refresh interval is accepted; FE also sends it explicitly for older BEs during rolling upgrades. - Publish converted properties only after successful file resolution, so a failed conversion cannot leave a partial cache. Preserve supported consumer, SASL, TLS, OAuth and AWS MSK IAM options. ### Release note Routine Load Kafka options are now restricted to an explicit allowlist. Unknown and executable options are rejected, and certificate paths must use files registered with `CREATE FILE`. Kerberos jobs must migrate from per-job keytabs/kinit commands to operator-managed ticket caches on eligible BEs. Existing jobs with rejected properties must be recreated with supported options. See `docs/routine-load-kafka-properties.md` for migration details. ### Check List (For Author) - Test: - [ ] Regression test: added broker-independent CREATE rejection cases; execution blocked locally by missing JDK 17. - [ ] Unit Test: added FE CREATE/ALTER, persisted-job and rolling-upgrade cases, plus BE property validation, managed-file and default-Kerberos cases. FE/BE test entry points were attempted but stopped at the JDK 17 environment check. - [x] Manual/static validation: FE Maven Checkstyle (zero violations), clang-format 16 verification, BE build-hygiene gates, and `git diff --check` passed. Checked all listed librdkafka options against pinned 2.11.0 source. Checked the affected Thrift definition on branch-3.1/4.0/4.1/4.2; no field IDs or wire formats change. - Full compilation and clang-tidy remain unverified: this checkout lacks third-party build dependencies and `compile_commands.json`. - Behavior changed: Yes; property acceptance and Kerberos ticket management are restricted as described above. - Does this need documentation: Yes; migration and supported-option policy are included in `docs/routine-load-kafka-properties.md`. No new network request path is introduced. Existing Kafka/OAuth endpoints remain configured by users holding the Routine Load LOAD privilege.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Problem Summary: Keep Routine Load per-job keytab login and renewal while rejecting executable Kafka configuration. Resolve managed keytabs, use the native ticket manager, and bind each Cyrus SASL connection to its consumer's explicit cache through the pinned librdkafka patch. Adapt refresh timing to ticket lifetime and retry transient refresh failures. ### Release note Managed FILE:keytab login and automatic renewal are supported on upgraded BEs. Custom kinit commands remain forbidden. Rebuild librdkafka with the updated Doris patch; older BEs cannot perform the new native login. ### Check List (For Author) - Test: Checkstyle, clang-format 16, build hygiene, diff checks and dependency patch dry-run passed. FE/BE/regression execution blocked by missing JDK 17; full compilation, clang-tidy and live Kerberos/Kafka tests remain unverified. - Behavior changed: Yes; managed keytabs use native login and per-consumer caches. - Does this need documentation: Yes; docs/routine-load-kafka-properties.md.
### What problem does this PR solve? Problem Summary: Remove the per-consumer credential binding patch from librdkafka. Keep native managed-keytab login and renewal functional by writing the operator-selected default Kerberos cache used by the unmodified client. Retain the existing shared-cache identity semantics. Serialize default-cache writers and do not delete the default cache when a refresh worker is released. Keep private-cache clients in a separate ticket-manager namespace. ### Release note This fix no longer modifies librdkafka or requires a special dependency rebuild. Managed keytabs retain automatic login and refresh without shell commands. The pre-existing multi-identity default-cache limitation remains documented. ### Check List (For Author) - Test: clang-format 16, build hygiene and git diff checks passed. Added default-cache login/refresh/lifetime and manager-key tests. BE UT entry point attempted but blocked by the Java environment (JDK 17 unavailable). Full compilation, clang-tidy and live Kerberos/Kafka tests remain unverified. - Behavior changed: Yes; native Kafka login writes the process default cache. - Does this need documentation: Yes; docs/routine-load-kafka-properties.md.
### What problem does this PR solve? Problem Summary: Remove the standalone Routine Load Kafka property document from this PR as requested. Keep configuration and upgrade notes in the PR description. ### Release note None ### Check List (For Author) - Test: No runtime test needed; documentation deletion only. git diff --check passed. - Behavior changed: No. - Does this need documentation: No; the requested standalone document is removed.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: two P1 recovery bugs and three P2 Kerberos regressions are reported inline. All 21 changed paths were reviewed at head 0b6f54d, including upstream/downstream callers. There were no existing inline threads or additional user focus points.
The review covers SQL property admission, effective FILE conversion, CREATE/ALTER, image and journal recovery, metadata and scheduled consumers, native Kerberos startup/renewal, ownership and mixed-version behavior. The two recovery findings are distinct: stale original CREATE properties can terminally cancel an already-migrated job, while historical ALTER validation can silently skip committed topic changes and leave a runnable job consuming the wrong source. The P2 findings cover synchronous KDC IO under the shared consumer-pool mutex, rejection of supported lowercase protocol values, and publication of canonicalized credentials under the requested alias instead of the authenticated client.
Critical checkpoint conclusions:
- Goal and proof: The shared allowlist and mandatory zero relogin interval implement the intended shell/property restrictions. The five inline findings prevent accepting the implementation as complete. Added tests establish intended cases by inspection; they were not executed.
- Scope and reuse: Changes are focused on Kafka admission and native ticket management; shared Thrift constants and the existing ticket manager avoid separate FE/BE allowlists and a new login framework.
- Concurrency and locks: Cache-local locking and the default-cache writer mutex have no identified new lock-order cycle. Native KDC login now runs under the shared consumer-pool lock, causing the P2 interference finding. FE job-lock/map behavior was traced.
- Lifecycle and initialization: The consumer closes librdkafka before releasing its retained ticket cache; manager cleanup and worker destruction stop/join refresh threads. Default-cache destruction preserves operator credentials, private/default manager keys differ, and the new function-local mutex avoids cross-TU initialization dependencies.
- Configuration: No server configuration knob is added. SQL property restrictions are intentional; ALTER and prepare still rebuild the effective map. Protocol-value compatibility has the P2 finding.
- Compatibility: Existing wire/persisted schemas are unchanged. Stricter property/file admission is an intentional behavior change. Managed-keytab login requires upgraded BEs; sending zero to older BEs suppresses their shell command but does not add native login, as declared by the author. The P1 recovery regressions require fixes.
- Thrift/protobuf: Compared the touched definitions with origin/branch-3.1, branch-4.0, branch-4.1 and branch-4.2. Only constants are added; TKafkaLoadInfo ids, types and requiredness remain unchanged, with no reused id or new required field.
- Parallel paths: CREATE/ALTER admission, prepare, metadata/offset/lag requests and scheduled consumers were covered. Both metadata and scheduled BE paths share whole-map validation. Recovery callers expose the two P1 findings; AWS property filtering remains connected.
- Conditional checks: New allowlist, managed-file and zero-only checks describe concrete rejection cases. Scalar FILE conversion cannot reenable the protected relogin interval. The protocol comparison unnecessarily narrows accepted values.
- Tests: Added FE/BE tests cover rejected/accepted properties, managed files, zero injection, cache-mode separation, login failure, lifetime-based intervals and worker reuse; regression cases reject during analysis without a broker. Recovery, blocked-login concurrency, lowercase protocol and returned-client canonicalization cases are missing and correspond to the findings.
- Result artifacts: No .out files are added or handwritten. New regression failures use test/sql/exception and preserve the table after setup.
- Observability: Validation identifies property keys and native login/refresh errors are logged. The replay catch logs an error while continuing with incomplete metadata, which is part of the P1 replay finding. No separate new metrics issue was verified.
- Persistence and failover: Current property maps and ALTER logs retain their existing format. Applying new admission policy to stale CREATE SQL and accepted historical ALTERs violates recovery equivalence as detailed inline.
- Writes and atomicity: Transaction publication, visible versions and delete bitmaps are untouched. Converted properties are published after complete FILE resolution. Pre-existing raw-map mutation on failed ALTER is distinct from the newly introduced durable-replay failure.
- FE/BE propagation: No new wire variable is required. Shared constants feed both implementations and explicit zero is sent through metadata protobuf maps and scheduled Thrift property maps.
- Performance: Property validation is bounded by the small per-job map. The material new cross-job delay is native KDC IO under the consumer-pool mutex. No additional verified large-allocation or hot-path regression was found.
- Additional risks: Default-cache discovery, private-cache callers, failed login publication, refresh due-time/retry, stop/join order, aliases and post-validation FILE transformations were rechecked. The new native path fails when a successful canonicalized AS reply returns a different client than the requested alias, as detailed in the fifth inline finding. The declared pre-existing multi-identity shared-cache limitation is not a new finding. Generic initialize/store publication concerns also exist in the pinned krb5 kinit path and were not asserted as a new regression.
Validation and limits: static inspection and primary dependency-source tracing only. No builds, tests, source edits or live Kafka/KDC experiments were performed, as required by this invocation. Live authentication, expiry/renewal, reconnect and KDC outage/recovery remain runtime validation limits. No unresolved static candidate or uncovered changed path remains.
Convergence: converged=true after 3 review rounds. All three latest-round reviewers returned NO_NEW_VALUABLE_FINDINGS on the same five-comment frozen set; required static coverage and candidate/risk dispositions are complete. Delivery verification is performed separately by the trusted helper and PR-scoped API readback.
Review converged after 3 round(s).
### What problem does this PR solve? Problem Summary: Keep the PR focused on FE/BE Kafka property allowlists and mandatory suppression of librdkafka shell commands. Remove native Kerberos login, automatic refresh, and ticket-cache adaptations and their tests. Keep the previously requested librdkafka and documentation removals intact. ### Release note Job-level keytabs and custom kinit commands are rejected. Kerberos jobs require operator-maintained ticket caches on eligible BEs; this security fix disables shell-based automatic login and renewal without adding a replacement worker. ### Check List (For Author) - Test: FE Checkstyle, clang-format 16, BE build hygiene and diff checks passed. FE/BE/regression entry points attempted; runtime tests blocked by missing JDK 17. Full compilation and live Kafka/Kerberos validation remain unverified. - Behavior changed: Yes; native auto-authentication is outside this PR's scope. - Does this need documentation: Upgrade requirements are in the PR description.
### What problem does this PR solve? Related PR: apache#69391 Problem Summary: Routine Load recovery reparses the original CREATE SQL to rebuild load expressions. An older job can retain a raw certificate path in that SQL even after ALTER has migrated its effective properties to CREATE FILE references. Reapplying the new data source admission checks to the stale CREATE SQL terminally cancels this otherwise valid job on FE restart. Use a recovery validation entry point that retains the existing database, table, load, job-property and merge checks without revalidating the original data source properties. CREATE admission and validation of the effective Kafka properties before runtime use remain enforced. ### Release note Kafka Routine Load jobs migrated to managed certificate files remain recoverable when their original CREATE SQL contains a raw certificate path. ### Check List (For Author) - Test: Added Gson round-trip coverage for migrated and unmigrated certificate properties, restored column descriptors, managed-file conversion, runtime rejection and unchanged CREATE admission. Checkstyle and git diff checks passed. run-fe-ut.sh was attempted but could not start tests because JDK 17 is unavailable; runtime behavior remains unverified locally. - Behavior changed: Yes; stale CREATE data source properties no longer cancel a persisted job during expression reconstruction. - Does this need documentation: No; restores intended recovery behavior.
sollhui
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
October 22, 2026 09:43
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes for the independently reconfirmed existing P1 historical ALTER replay failure. No additional verified inline findings. Review covers all eleven changed paths at b7ef8bc.
The new validation can throw during replay after the logged custom-property map is merged but before topic, broker, offsets and job settings are applied. Replay catches and logs the failure. A later logged interval=0 repair can make the incomplete state runnable against the original topic. This remains the existing finding and is not reposted.
The existing stale-CREATE recovery P1 (4346548947) is addressed: recovery now reconstructs expressions through validateForRecovery without analyzing the original data-source map, while runtime conversion checks current effective properties. The three native-Kerberos P2 threads (4346548959, 4346548965, 4346548968) concern code absent from this head and no longer apply.
Applicable critical checkpoint conclusions:
- Goal and proof: Shared FE/BE property policy, managed certificate references and mandatory zero relogin are implemented on inspected paths. Added unit and regression cases cover admission, persisted-map rejection and recovery; historical ALTER replay still violates durable-state correctness.
- Scope and reuse: The reusable BE helper and generated shared sets keep policy focused; expression recovery is separated from CREATE admission. No unrelated source changes identified.
- Concurrency and locks: Existing scheduler/task/metadata threads use existing job, pool and consumer locks. Added checks perform local property lookups; no new lock order, thread or blocking network operation is introduced. Existing FILE/AWS work under locks predates this change.
- Lifecycle, memory and static initialization: Configuration cleanup remains guarded by Defer. Conversion publishes a complete temporary map; resume rebuilds it through prepare before task creation. No new ownership cycle, cache, significant allocation or cross-translation-unit initialization dependency was identified.
- Configuration: No Doris process configuration is added. SQL property changes follow the existing paused-job ALTER flow. External Kerberos ticket provisioning and renewal are an explicit operational requirement in the PR release note.
- Compatibility: Rejected legacy options, managed-file requirements and disabled automatic Kerberos login are intentional documented behavior changes. New FE sends zero to old BE; new BE independently sets zero for old FE. Old FE with old BE retains prior behavior. No persisted format or function ABI change identified.
- Thrift/protobuf: Only three string-set constants are added. Compared the affected area on HEAD and origin/branch-3.1, branch-4.0, branch-4.1 and branch-4.2; no field/enum IDs are modified or reused and no required field is added.
- Parallel paths: CREATE, ALTER and effective-map runtime checks were traced. Task submission and partition, timestamp-offset, real-offset and latest-offset metadata requests use the converted map and common BE initialization. Kinesis does not use the Kafka helper; common recovery only consumes reconstructed load expressions.
- Conditions and errors: Unknown keys, nonzero interval and unmanaged/empty file references have concrete rejection paths and comments. New BE setting failure returns non-OK Status, and FE analysis errors become DdlException. The replay exception handling remains the confirmed P1. Pinned librdkafka configuration and Cyrus initialization support the zero-setting behavior.
- Test coverage: FE cases cover CREATE/ALTER, malformed zero values, all managed-file keys, normal/AWS options, populated-cache rejection and migrated recovery. BE cases cover full-map rejection before file/client effects and mandatory zero. Regression cases cover broker-independent CREATE rejection. Live authentication, managed-file consumption, mixed-version execution and historical ALTER replay are not established by these tests.
- Test results: No golden output files are changed. New negative regression cases use test/sql/exception, hardcode the table name and drop before use. Test sources were inspected; no tests or builds were executed under this review contract.
- Observability: New errors identify property keys and give corrective guidance without copying values. Existing pause/error paths provide runtime diagnostics; no new distributed operation requiring separate metrics was found.
- Persistence and failover: Current effective properties retain existing persistence; stale original SQL no longer cancels migrated jobs. Historical ALTER replay remains inequivalent to the durable transition, including after promotion/restart, as described in the existing P1.
- Data writes and atomicity: Row transaction, visible-version and delete-bitmap algorithms are unchanged. The replay issue can redirect resumed ingestion to the wrong source. The ordinary failed-ALTER source-map mutation predates this PR; retained conversion cache did not establish a separate new runnable-state bug.
- FE/BE variables: Existing property maps carry FILE:id:md5 and explicit zero on all traced producers; no new scalar/query-plan variable or additional sending path is required.
- Performance and build integration: Validation scales with the small property map at consumer initialization; no per-message work or new RPC is added. Recursive CMake discovery includes new helper/tests; no new hub/PCH include, explicit-template family or newer glibc API is introduced. Compilation and final ELF ABI were not exercised.
- Additional recheck: Revisited failed conversion/resume, pool reuse, AWS filtering, offsets, stale CREATE recovery and replay. No additional verified finding or unresolved code candidate remains.
No additional user focus was supplied. Coverage is complete for static review; runtime, build and integration behavior remain unverified. The existing P1 continues to block approval.
Review converged after 1 round(s).
Existing P0/P1 findings confirmed for this head: #69391 (comment)
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28416 ms |
Contributor
TPC-DS: Total hot run time: 150236 ms |
Contributor
ClickBench: Total hot run time: 25.02 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28147 ms |
Contributor
TPC-DS: Total hot run time: 150196 ms |
Contributor
ClickBench: Total hot run time: 25.28 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
3 similar comments
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
…y test ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69391 Problem Summary: The Routine Load display regression passed sasl.jaas.config, which the Kafka property admission lists reject before creating a job. Its unconditional finally cleanup then stopped a nonexistent job and hid the original creation error. Remove the rejected property and its assertions, retain password masking checks for SHOW and information_schema, and create the job before entering try/finally. Existing FE unit coverage continues to check legacy JAAS masking. ### Release note None ### Check List (For Author) - Test: git diff --check passed. Verified that every custom Kafka property in the changed block is allowed by the current Thrift admission lists. Attempted ./run-regression-test.sh --run -d load_p0/routine_load -s test_show_routine_load; it exited before running tests because JDK 17 is unavailable locally. - Behavior changed: No; regression test only. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes for the previously reported historical ALTER replay defect (4346548956), which still applies to head 28d9ed9. Replay calls the new runtime validator before applying all logged topic/broker/offset/job changes; rejecting an old property can therefore lose a committed transition. This finding is carried forward without posting it again. One new P2 identifies an existing positive SSL unit test that the new policy deterministically breaks.
The other four existing findings are addressed at this head: recovery now reconstructs expressions without validating stale original CREATE properties, and the native Kerberos implementation responsible for the three BE findings has been removed. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: shared FE/BE allowlists and mandatory zero relogin cover the property admission/client creation paths. Zero bypasses both initial and recurring kinit timers in the pinned librdkafka implementation. Tests were inspected, not executed.
- Scope and reuse: changes are focused on Kafka property validation, shared constants, recovery reconstruction and corresponding tests; existing managed-file and client plumbing is reused.
- Concurrency: reviewed scheduler/ALTER/lag job locks and BE pool/consumer locks. No new threads, lock order or synchronous KDC operation remains; validation adds a small map scan.
- Lifecycle and static initialization: conversion publishes a complete temporary map; Conf cleanup covers errors. Generated constants are accessed during execution without a new cross-translation-unit initializer dependency. No new ownership leak found.
- Configuration: no new operator configuration or dynamic reload requirement. The policy intentionally requires managed certificate files and externally maintained default Kerberos tickets.
- Compatibility: new FE maps explicitly send zero to older BEs; new BEs enforce it for older FE maps. Rejected historical properties require migration. Historical ALTER replay remains incorrect as described above.
- Thrift/protobuf: inspected the affected definitions on HEAD and branch-3.1/4.0/4.1/4.2; additions are named sets only, with no field ID, enum, requiredness or wire-layout change.
- Parallel paths: traced CREATE, ALTER, recovery, metadata probes, scheduled tasks, consumer reuse and AWS handling. Kinesis does not enter Kafka validation; cloud/non-cloud transport paths remain consistent.
- Conditions: rejection, managed-file, mandatory-zero and stale-CREATE recovery branches have concrete explanations and propagate errors.
- Test coverage: new FE/BE tests cover forbidden keys, interval values, managed files, defaults, AWS and recovery; regression tests cover analysis failures. The existing positive SSL fixture still needs migration (new inline finding). Replay and real mixed-version authentication lack runtime coverage here.
- Test results: no .out changes; new error cases use test/sql/exception. SHOW masking removes the now-rejected JAAS input. No build, test, formatter or runtime validation was run under this static-only contract.
- Observability: new errors identify offending keys without property values and reach job pause/error reporting. Replay currently logs the failure while skipping durable state, which is the confirmed existing P1.
- Persistence and failover: effective properties retain their existing serialization and recovery avoids stale CREATE admission checks. ALTER log replay equivalence fails; no new persisted fields were added.
- Writes and atomicity: ingestion transactions, rowset visibility and delete bitmaps are unchanged. Conversion avoids partial-cache publication; the inherited live-ALTER mutation-before-failure behavior does not justify a duplicate finding here.
- FE/BE passing: both task Thrift and metadata protobuf requests use the existing property map, including explicit zero; no new transmitted variable is missing a send path.
- Performance and memory: bounded property-map work occurs before client construction/conversion, with no per-row work, bulk allocation or new background cache. No additional performance defect was established.
- Remaining scope: all 12 changed files and the identified upstream/downstream risks were reviewed. No concrete candidate remains unresolved. Compilation, service authentication, rolling upgrades and final ELF compatibility are unverified because builds/tests were prohibited.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #69391 (comment)
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Related PR: apache#69391 Problem Summary: The Kafka property policy requires managed file references, but testSslConfigWithoutSasl still uses raw certificate paths and fails during analysis. Replace its three certificate paths with FILE:<name> references. ### Release note None ### Check List (For Author) - Test: git diff --check passed. FE unit test not run because the worktree lacks thirdparty/installed/bin/protoc. Checkstyle attempted but blocked by JVM initialization failure. - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes for the independently reconfirmed existing P1 historical ALTER replay defect. No additional verified inline findings. Reviewed all 13 changed paths at 970880e.
Replay merges the logged custom properties and calls the new runtime validator before restoring topic, broker, offsets and job settings. Rejection of a historical property therefore skips part of a committed transition. A later logged property repair can make the incomplete state runnable against the wrong topic. This remains the existing finding and is carried forward without a duplicate inline comment.
The stale-CREATE recovery P1 (4346548947) is addressed by validateForRecovery, which reconstructs load expressions without revalidating original data-source properties. The positive SSL fixture P2 (4356220741) now uses managed FILE references. The three native-Kerberos P2s (4346548959, 4346548965, 4346548968) concern implementation removed from this head. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: Shared FE/BE allowlists, managed certificate references and mandatory zero relogin implement the intended client-property policy on the inspected paths. The existing replay P1 prevents approval. Added tests express the admission and recovery contracts; they were inspected, not executed.
- Scope and reuse: Changes are focused on Routine Load Kafka policy, recovery and tests. Shared Thrift constants and the existing managed-file/client infrastructure avoid divergent policy copies and new authentication machinery.
- Concurrency and locks: Traced FE scheduler, ALTER, task and lag access plus BE pool/consumer locks. Validation adds local map lookups without a new thread, lock order or synchronous KDC operation. Existing FILE/AWS operations under locks are inherited.
- Lifecycle and initialization: Conf cleanup remains guarded on error. Conversion publishes a complete temporary map; prepare rebuilds before scheduled task creation. No new ownership cycle, significant untracked allocation or cross-translation-unit initialization dependency was identified.
- Configuration: No process configuration is added. SQL properties follow the existing paused-job ALTER flow. Externally provisioned and renewed Kerberos tickets and rejection of legacy options are explicit release-note requirements.
- Compatibility: New FE conversion sends zero to older BEs; new BE initialization enforces zero for older FEs. Old FE with old BE retains previous behavior. Property restrictions are documented behavior changes. Existing persisted formats and function interfaces are unchanged; historical ALTER replay remains incompatible with the durable transition.
- Thrift/protobuf: Compared the touched area with origin/branch-3.1, branch-4.0, branch-4.1 and branch-4.2. Only named string-set constants are added; no field/enum ID, type, requiredness or wire layout is changed.
- Parallel paths: Covered CREATE, ALTER, original-SQL recovery, preparation, partition/offset/lag probes, scheduled consumers, consumer reuse and AWS filtering. Metadata protobuf and task Thrift maps reach common BE initialization. Kinesis does not use Kafka validation. The inherited early ALTER offset probe can see an empty transient map after recovery; this does not establish a distinct change-induced defect.
- Conditions and errors: Unknown keys, nonzero interval and unmanaged file references have concrete rejection cases. Mandatory BE setting failure returns non-OK Status; FE runtime conversion wraps validation failures in DdlException. Zero skips both initial kinit and refresh timer in the pinned librdkafka Cyrus implementation. The replay catch remains the confirmed error-handling defect.
- Test coverage: FE cases cover CREATE/ALTER rejection, all managed-file keys, normal/AWS options, explicit zero, populated-cache rejection and migrated recovery. BE cases cover complete-map rejection before file/client effects and mandatory zero. Regression cases cover broker-independent admission errors. Historical ALTER replay and real Kafka/Kerberos/mixed-version execution are not proved by these tests.
- Test results: No golden output file changes. New negative regressions use test/sql/exception, hardcode their table and drop before use. The SHOW masking fixture removes the newly rejected JAAS setting. No build, test, formatter or runtime validation was executed under this static-only contract.
- Observability: New validation errors name the property and remediation without copying its value. Existing job pause/error paths expose failures. Replay logging followed by continuation with incomplete state is part of the existing P1; no separate metrics change is required by a new operation.
- Persistence and failover: Effective property serialization and ALTER logging retain their formats. Recovery preserves migrated jobs, but historical ALTER replay can lose committed state on restart or promotion as detailed above.
- Writes and atomicity: Row transaction publication, visible versions and delete bitmaps are untouched. Complete-map publication avoids partial conversion caches. Source-map mutation on a rejected live ALTER predates this PR; no separate new runnable-state defect was established. The replay finding can redirect subsequent ingestion.
- FE/BE propagation: Existing property maps carry converted FILE:id:md5 references and explicit zero through the traced task and metadata producers. No new query-plan or scalar wire variable requires additional sending paths.
- Performance and build integration: New validation scales with the small per-job map and adds no per-message work or RPC. Recursive CMake discovery includes the helper and test. No hub/PCH header, explicit-template family or newer libc API is introduced. Compilation, final ELF ABI and runtime performance remain unverified.
- Additional recheck and limits: Rechecked failed conversion/resume, retained cache, offsets, pool matching, AWS filtering, release definitions and recovery transitions. No additional substantiated candidate or uncovered changed path remains. Static review is complete; live authentication, file consumption, rolling upgrades and build behavior remain validation limits.
Review converged after 3 round(s).
Existing P0/P1 findings confirmed for this head: #69391 (comment)
### What problem does this PR solve? Related PR: apache#69391 Problem Summary: testRecoverMigratedCertificateProperties mocks the internal catalog but omits the CatalogMgr used by ConnectContext.setDatabase during RoutineLoadJob deserialization. The resulting null pointer cancels the job before the recovery assertions can run. Mock the catalog manager, catalog name, and database lookup by name so recovery uses consistent test metadata. ### Release note None ### Check List (For Author) - Test: git diff --check passed. KafkaPropertyValidationTest.java passed Checkstyle 9.3 with the repository configuration. FE UT not run because thirdparty/installed/bin/protoc is missing; Maven reactor validation also cannot resolve uncached dependencies offline. - Behavior changed: No - Does this need documentation: No
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes for the existing historical ALTER replay P1 at head db800308b50d30ffdcd0446b9a5be9a025ce36f7. No distinct new inline findings were established across all 13 changed files.
Existing finding 4346548956 still applies: replayModifyProperties() invokes modifyPropertiesInternal(), which merges the logged properties and runs the new validation before applying the remaining logged topic, broker, offset and job-setting changes. A historical ALTER changing topic A to B with relogin=60000 is rejected during replay; a later ALTER restoring relogin=0 can succeed while leaving the job pointed at A. Replay must apply the complete durable transition independently of runtime admission policy. This finding is carried forward without reposting it.
The previous CREATE-recovery P1 (4346548947) is fixed by validateForRecovery(), and the positive SSL fixture issue (4356220741) is fixed by managed FILE references. The three native-Kerberos findings (4346548959, 4346548965, 4346548968) no longer apply because that implementation was removed. Externally maintained Kerberos tickets and rejection of historical unsupported options are deliberate behavior changes documented in the current PR description.
| Critical checkpoint | Conclusion |
|---|---|
| Goal and evidence | Shared FE/BE allowlists, managed certificate paths and shell suppression implement the intended restriction; historical ALTER replay remains the blocker above. Added tests were inspected, not executed. |
| Scope and reuse | Changes stay within Kafka property admission/conversion, recovery and associated tests; FE and BE reuse the same generated policy constants. |
| Concurrency and locks | FE job locks and BE consumer/pool mutexes retain existing ordering. The new validation is bounded local work; no new synchronous KDC operation or lock inversion was found. Existing file/client setup under the pool lock predates this PR. |
| Lifecycle and initialization | Converted properties remain transient and are rebuilt before scheduling. A complete temporary map is published after FILE resolution; existing Conf cleanup covers rejection. No new static-initialization dependency or ownership cycle was found. |
| Configuration | No new dynamic process configuration. The property policy is explicit and relogin accepts only literal zero. |
| Compatibility and rolling upgrades | No wire/storage layout change. New BEs validate older FE input and force zero; successful new-FE conversion sends zero to older BEs. Automatic Kerberos login/renewal removal is disclosed. Historical ALTER replay is still incompatible as described above. |
| Thrift/protobuf | Only three constant sets were added. Compared touched definitions with branch-3.1, branch-4.0, branch-4.1 and branch-4.2; no field-ID, enum-value, requiredness or constant-definition collision was found. |
| Parallel paths | Reviewed CREATE, ALTER, recovery, metadata/offset probes, scheduled tasks, cloud/non-cloud selection, AWS and generic SASL/TLS. Kinesis does not use librdkafka. Successful conversions and scheduled task maps contain zero; a pre-existing restored-PAUSED ALTER-offset route can send an empty transient map, which also lacks user SASL/principal settings. No new defect was established there. |
| Special conditions and errors | Exact-zero and managed-file checks have concrete rationale. BE validates the entire map before user setters or file access; new Status/exception results propagate. The replay catch-and-continue defect is already covered above. |
| Test coverage | FE/BE unit definitions cover rejected and accepted properties, managed files, mandatory zero, cached-map validation, recovery and rejection before side effects. Regression definitions cover broker-independent analysis failures and retain password masking. Historical ALTER replay and failed-rebuild cache scenarios lack dedicated tests. |
| Expected results | No golden output files changed. New exception expectations and updated positive fixtures agree with the intended policy on static inspection. |
| Observability | Rejection errors identify the key without echoing its value; existing job/PAUSE and metadata error context remains. No new asynchronous subsystem requires additional metrics. |
| Persistence and failover | Original CREATE reconstruction is repaired; historical ALTER transition equivalence remains broken under the confirmed existing P1. No new persisted fields were added. |
| Writes, crashes and atomicity | Commit/publish, visible-version and delete-bitmap logic is untouched. New cache publication avoids partial converted maps. Failed-ALTER mutation of the source map predates the PR; preparation rebuilds before ingestion, so no distinct new write/atomicity finding was established. |
| FE/BE variable passing | The existing property map carries the safe relogin value through protobuf metadata requests and Thrift tasks; no additional wire field is needed. |
| Performance and memory | Validation scans a small property map; no new row-processing work, substantial allocation, worker or retained resource was introduced. No substantiated performance regression was found. |
| Other checks | New BE source/test registration is covered by existing CMake globs. The header uses forward declarations; no PCH, extern-template, unity-source inclusion or new libc API issue was found. No optimizer rewrite or nullable-column behavior changed. |
There was no additional user-provided focus. Coverage is complete for this static review, with no unresolved static candidate or risk question. No builds, tests, live Kafka/TLS/Kerberos checks, or release-artifact ABI checks were run; runtime behavior and compilation remain unverified.
Review converged after 2 round(s).
Existing P0/P1 findings confirmed for this head: #69391 (comment)
### What problem does this PR solve? Related PR: apache#69391 Historical ALTER replay ran the current Kafka property policy and file conversion before applying topic, broker, offset and job-setting changes. Rejecting an option accepted by an older FE could therefore leave a partially restored committed transition. Keep live ALTER preparation separate from applying persisted properties. Replay restores the Kafka fields without resolving files, validating current Kafka policy, checking runtime partitions or repeating cloud progress RPCs. Invalidate the converted cache and enforce current policy before preparation and metadata requests, including ALTER offset lookups. Prepare live custom properties in a temporary map before publishing them. ### Release note Preserve complete historical Routine Load ALTER state during recovery. Jobs with unsupported Kafka properties remain recoverable and are paused with an error when scheduled under the current policy. ### Check List (For Author) - Test: Add FE tests for historical ALTER and subsequent repair in cloud/non-cloud modes, deferred file resolution, scheduler pause reporting, metadata admission and live ALTER publication. Targeted FE unit tests attempted through run-fe-ut.sh but blocked before execution by unavailable JDK 17. Checkstyle reports zero violations; git diff --check passed. Full compilation remains unverified; thirdparty/installed/bin/protoc is absent. - Behavior changed: Yes; journal replay applies committed Kafka state without runtime preparation or repeated cloud progress reset RPCs. - Does this need documentation: No; the existing PR upgrade requirements remain applicable.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Request changes: two P1 and two P2 findings are attached inline. A job-only ALTER can corrupt a datetime default offset and skip records in newly discovered Kafka partitions; the new FE test also imports two nonexistent classes. Separately, repairing persisted properties together with offsets is rejected before the repair is merged, and the positive ALTER test omits an automatically inserted property from its expected map.
Reviewed all 13 changed paths and their CREATE/ALTER, recovery/replay, scheduler, metadata, task and consumer-pool interactions at commit 41c559f. This was static review only: no builds, tests, broker authentication, native ticket-cache behavior or final ELF ABI checks were executed.
Critical checkpoint conclusions:
- Goal and proof: the shared allowlist, managed certificate references and mandatory zero Kerberos interval reach FE admission and BE initialization. The attached correctness and test defects prevent considering the implementation complete; added tests were inspected, not run.
- Scope and reuse: changes remain focused on Routine Load property policy and recovery/preparation, with shared Thrift constants and a reused FE validator.
- Concurrency and locks: SQL ALTER, scheduler, task and lag paths share job state. The new offset snapshot uses the job lock and performs Kafka RPC afterward. Existing cloud reset/journal ordering remains; no additional deadlock was established.
- Lifecycle and initialization: FILE conversion publishes a complete local map and replay invalidates transient conversion state. Reprocessing an existing cache causes the reported datetime-default corruption. No new thread, ownership cycle or cross-translation-unit static initializer dependency was found.
- Configuration: no server configuration knob is added. The allowlist and external Kerberos ticket-management requirement deliberately change job behavior and are described in the PR release/upgrade notes.
- Compatibility: new FE conversions explicitly send interval zero to older BEs, and new BEs enforce zero for older FEs. Rejected historical properties are preserved on replay and checked before runtime use. The reported repair-plus-offset ordering is an avoidable migration limitation.
- Thrift/protobuf: HEAD and origin/branch-3.1, branch-4.0, branch-4.1 and branch-4.2 definitions were compared. Only three constant sets are added; field IDs, enum values, optionality and persisted formats do not change.
- Parallel paths: Kafka partition enumeration, timestamp/symbolic/latest offset metadata, scheduled tasks and asynchronous commits share the consumer initialization gate. Kinesis expression recovery still executes the common load-description reconstruction and retains its separate source policy.
- Conditions and comments: the new checks explain concrete forbidden-property, file-resolution and recovery scenarios. Required failures propagate through AnalysisException/DdlException or checked Status returns.
- Test coverage: new FE/BE negatives cover CREATE/ALTER, restored maps, managed files, rolling-version settings and rejection before file/client side effects; recovery/replay and both Groovy suites were reviewed. The datetime-default/job-only-ALTER/new-partition sequence is missing, and the two attached test defects need correction.
- Expected results: no regression output files change. Negative Groovy cases need no result artifact; the positive FE ALTER assertion has the reported missing default-offset entry.
- Observability: policy failures identify property names without values, and existing scheduler PAUSED reasons/logging expose rejection. No new high-volume metric/log path requires an additional finding.
- Persistence and failover: historical ALTER replay now applies all committed fields without current policy validation, FILE lookup or cloud reset. Image recovery reconstructs expressions without revalidating stale CREATE source properties. The earlier two P1 threads are addressed; the new cache bug can still affect subsequent partition progress.
- Writes and atomicity: failed FILE resolution does not publish partial raw/cache updates. The new default-offset corruption violates ingestion correctness. No new storage transaction, visible-version or delete-bitmap algorithm is introduced; existing cloud reset-before-journal behavior is unchanged.
- FE/BE propagation: both Thrift tasks and protobuf metadata carry the converted map; empty maps also receive interval zero. Exact map/broker/topic equality fences consumer reuse, and new production/test sources are covered by existing CMake globs.
- Performance and memory: validation is bounded by the small configuration map; no significant new allocation or hot data-processing loop is introduced. Existing conf cleanup covers early errors; no separate memory/performance finding was substantiated.
- Other risks and scope limits: all initial risk questions and candidates were accepted, deduplicated or dismissed with code evidence. Compilation, live Kafka/Kerberos integration and release-artifact ABI remain unverified under this static-only contract.
Existing-thread disposition: 4346548947 and 4346548956 are addressed by recovery-only expression validation and side-effect-free durable replay; 4356220741 is addressed by managed FILE fixtures. The three native-Kerberos threads (4346548959, 4346548965, 4346548968) concern code removed from this head. None remains an independently confirmed existing P0/P1, and none is reposted. There was no additional user-provided focus.
Review converged after 2 round(s).
| modifyPropertiesInternal(jobProperties, dataSourceProperties); | ||
| Map<String, String> convertedProperties = prepareModifyProperties(dataSourceProperties); | ||
| applyProperties(jobProperties, dataSourceProperties); | ||
| setConvertedCustomProperties(convertedProperties); |
Contributor
There was a problem hiding this comment.
[P1] Preserve datetime defaults when reusing the conversion cache
For a job created with a datetime property.kafka_default_offsets, the first conversion removes kafka_origin_default_offsets but leaves the numeric kafka_default_offsets in the cache. A job-only ALTER (for example, changing max_batch_rows) returns that existing cache from prepareModifyProperties(), so this unconditional setter extracts the remaining millis string into kafkaDefaultOffSet. convertedDefaultOffsetToLong() then falls back to OFFSET_END. If a partition is added while the job is paused and the job resumes, the scheduler updates partition progress before prepare() rebuilds the cache, records that partition's end offset, and skips its records since the configured datetime. Extract defaults only from a freshly resolved map, or preserve the derived default when reusing the cache.
| import org.apache.doris.common.DdlException; | ||
| import org.apache.doris.common.Pair; | ||
| import org.apache.doris.common.jmockit.Deencapsulation; | ||
| import org.apache.doris.common.util.KafkaUtil; |
Contributor
There was a problem hiding this comment.
[P1] Import the existing Kafka utility and ALTER command classes
This new test imports org.apache.doris.common.util.KafkaUtil and org.apache.doris.nereids.trees.plans.commands.load.AlterRoutineLoadCommand, but neither class exists in those packages on this head. The actual classes are org.apache.doris.datasource.kafka.KafkaUtil and org.apache.doris.nereids.trees.plans.commands.AlterRoutineLoadCommand, as used by KafkaRoutineLoadJob. Both types are referenced by these tests, so FE test compilation fails before any of the new validation/recovery tests can run. Correct both imports.
| try { | ||
| // Recovery leaves the conversion cache empty. Validate the effective | ||
| // properties before an ALTER offset lookup can contact an older BE. | ||
| convertCustomProperties(false); |
Contributor
There was a problem hiding this comment.
[P2] Merge repairing ALTER properties before validating offset lookups
An older paused job with ssl.ca.location=/etc/ca.pem can be migrated to FILE:ca.pem, but adding kafka_partitions=0 and kafka_offsets=100 to that same valid ALTER makes this call reject the old raw-path map. modifyProperties() invokes convertOffset() before prepareModifyProperties() merges the replacement, so the repair never reaches validation. This also occurs for explicit positive offsets, where KafkaUtil.getRealOffsets() needs no BE request at all. Prepare and validate the merged effective properties first, then use that safe snapshot for any required offset lookup while preserving atomic publication.
| EditLog editLog = Mockito.mock(EditLog.class); | ||
| Mockito.when(env.getEditLog()).thenReturn(editLog); | ||
| job.modifyProperties(command); | ||
| Assertions.assertEquals(Map.of("property.sasl.kerberos.min.time.before.relogin", "0"), |
Contributor
There was a problem hiding this comment.
[P2] Include the analyzed default offset in the ALTER expectation
After correcting the imports, this assertion still fails: safe.analyze() calls analyzeKafkaDefaultOffsetProperty(), which inserts kafka_default_offsets=OFFSET_END even for this ALTER. applyProperties() copies that entry into the raw custom map, and getCustomProperties() returns it as property.kafka_default_offsets; only the converted map removes the bookkeeping key. The actual map therefore has two entries, while this assertion expects one. Include the inserted default-offset property, or assert the intended entry without comparing against an incomplete map.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Routine Load forwards SQL-supplied Kafka properties to librdkafka without restricting the configuration surface. Some options execute shell commands or load executable modules. This PR limits the properties exposed to SQL and disables librdkafka's shell-based Kerberos login/renewal.
Conf::set(), including requests from older FEs.sasl.kerberos.kinit.cmd,plugin.library.paths, SSL engine/provider settings, unknown properties, and job-levelsasl.kerberos.keytab. Certificate/key/CA/CRL/keystore paths must reference files registered with CREATE FILE.sasl.kerberos.min.time.before.reloginto be the literal0and force it to0on BE. Disabling custom commands alone is insufficient because librdkafka's default command also interpolates user-controlled configuration. FE sends the zero setting explicitly to protect new-FE/old-BE requests.This PR contains no native Kerberos login/refresh implementation, ticket-cache changes, librdkafka patch, or standalone documentation file. Native automatic authentication is outside its scope.
Release note
Routine Load Kafka options are restricted to an explicit allowlist. Unknown/executable properties and job-level keytabs are rejected. Certificate file paths require CREATE FILE references.
Kerberos automatic login and renewal through librdkafka's shell command stop with this change. Operators must provision and renew a default Kerberos ticket cache accessible to every BE eligible to run these jobs. The principal and service-name properties remain available. This PR does not restore automatic ticket refresh.
Existing jobs containing rejected properties must be recreated with supported options, because ALTER merges properties and cannot remove an existing rejected key. Coordinate ticket-cache provisioning with the upgrade to avoid interrupting Kerberos jobs. New FEs disable shell login on older BEs as well; an old FE sending to an old BE retains its previous behavior until at least one endpoint is upgraded.
Check List (For Author)
No Thrift field IDs, enum values, or persisted formats change. The affected definition was checked against branch-3.1/4.0/4.1/4.2.
No new network request or artifact-loading path is introduced. Existing Kafka/OAuth endpoints and managed-file transport remain in use. Routine Load requires LOAD privilege; CREATE FILE requires ADMIN.