[fix](catalog) safely publish Hadoop properties - #66392
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes.
The local result map and final volatile assignment fix the reported early-publication window, preserve successful aggregation/fingerprint semantics, and improve retry behavior after initialization failures. However, two production concurrency gaps remain: a concrete Paimon caller still mutates the published cache while other paths iterate or copy it, and the getter can return null when invalidation lands between its volatile reads. The new regression test also has a scheduling window that can pass against the pre-fix code.
Critical checkpoints:
- Correctness and concurrency: two blocking issues are detailed inline.
- Lifecycle and invalidation: aggregation, monitor reentrancy, reset ownership, and exception retry were traced; no other issue was found.
- Compatibility, performance, and security: no additional regression was found by static review.
- Tests: JUnit and Mockito compatibility plus bounded cleanup hold, but the old-code failure oracle needs strengthening.
- User focus: no additional focus was provided; the full PR was reviewed.
- Review constraints: no repository code-review skill or required AGENTS.md was present. Builds and tests were not run because the authoritative review prompt forbids them.
- Completion: the three-round full and risk-focused review converged on this frozen payload with no unresolved candidate points.
| hadoopProperties = new HashMap<>(); | ||
| // Publish the volatile cache only after construction because readers skip this | ||
| // lock once it is non-null and must never observe a map still being mutated. | ||
| Map<String, String> result = new HashMap<>(); |
There was a problem hiding this comment.
[P1] Return a retained snapshot across invalidation
Please use the local-snapshot form for the whole double-checked getter and return that snapshot. The final return hadoopProperties is another volatile read: a concurrent addProperty/modifyCatalogProps can run after this method's first read, acquire this monitor, reset the field to null, and make this call return null (the same window exists just after initialization releases the monitor). Current consumers immediately iterate or copy the result, so that becomes an intermittent NPE during catalog updates.
| } | ||
| StorageProperties.setCombinedFsCacheKey(hadoopProperties, storageMap.values()); | ||
| StorageProperties.setCombinedFsCacheKey(result, storageMap.values()); | ||
| hadoopProperties = result; |
There was a problem hiding this comment.
[P1] Keep the published cache immutable
The construction itself is safely published now, but this still exposes the same mutable HashMap after publication. PaimonTableValuedFunction keeps this exact map (PaimonTableValuedFunction.java:89-90) and appends Kerberos entries in appendHMSKerberosProps, while ExternalCatalog.buildConf iterates it and PaimonWriteBinding copies it. Those paths can still race into partial Kerberos state or ConcurrentModificationException, and the TVF-specific entries also leak into the catalog cache. Please keep the cached snapshot encapsulated or immutable and copy it before caller-specific augmentation.
| Assert.assertTrue(iterationStarted.await(5, TimeUnit.SECONDS)); | ||
|
|
||
| Future<Map<String, String>> concurrentReader = executor.submit(() -> { | ||
| readerStarted.countDown(); |
There was a problem hiding this comment.
[P2] Make the old-code failure handshake deterministic
readerStarted only proves that the task began, not that it reached getHadoopProperties(): the worker can be descheduled immediately after this countdown. In that schedule the 200 ms get times out even on the old early-publication code; after allowIteration is released both futures return the complete map and the regression passes falsely. Please use a bounded handshake that observes the reader complete on the old path or actually become blocked at the getter or monitor before releasing the initializer.
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Approval opinion: the current head addresses the previously reported blockers, and I found no new issue that warrants an inline comment.
The getter now retains one observed snapshot across invalidation, constructs and wraps the map before volatile publication, and never exposes the mutable backing map. The Paimon metadata TVF copies that immutable snapshot before adding caller-specific Kerberos properties. The revised concurrency test uses the initializer-held monitor as a stable BLOCKED handshake, so the old early-publication path terminates and fails while the fixed path remains blocked until the completed map is published.
Critical checkpoints:
- Correctness and concurrency: double-checked publication, monitor reentrancy, volatile visibility, concurrent invalidation, exception retry, and storage-generation ordering were traced; no changed-line defect remains.
- Lifecycle and compatibility: every direct and indirect
getHadoopProperties()consumer was audited. Mutation sites now copy or merge into private maps, and the remaining callers only iterate, retain, or serialize the immutable snapshot. - Configuration, performance, and security: aggregation and the combined filesystem cache key are preserved; the only added steady-state work is the necessary per-TVF copy; no security or credential-leak regression was found.
- Tests: the old/fixed schedules, timeout bounds, result publication, and cleanup paths were reviewed. Builds and tests were not run because the authoritative review prompt forbids them.
- Existing review context: the retained-snapshot, immutable-cache/Paimon-copy, and deterministic-handshake threads are all addressed at
edc0a8f21b5dd4415665a1786884a08278b4e7c4. A separate cross-accessor ALTER interleaving was investigated and dismissed because the same ordering exists in the base revision and this patch removes alias leakage without worsening it. - User focus: no additional focus was provided; the complete PR was reviewed.
- Review constraints: no repository
code-reviewskill or requiredAGENTS.mdfile was present, so the authoritative prompt was used as the fallback contract. - Completion: two full normal-review rounds and their separate risk-focused scans converged on this frozen zero-inline comment set with no unresolved candidate points.
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
### What problem does this PR solve? Issue Number: None Related PR: apache#66392 Problem Summary: Master replaced the legacy Hadoop property cache with a shared storage adapter snapshot. Preserve the original fix invariant by publishing an immutable type-keyed map so connector consumers cannot mutate catalog-wide state after publication. Add deterministic coverage for atomic publication and snapshot immutability. ### Release note Prevent connector consumers from modifying shared catalog storage adapter snapshots. ### Check List (For Author) - Test: Unit Test (`CatalogPropertyTest`) - Behavior changed: No. This hardens the existing snapshot contract. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66392 Problem Summary: Master replaced the legacy Hadoop property cache with a shared storage adapter snapshot. Preserve the original fix invariant by publishing an immutable type-keyed map so connector consumers cannot mutate catalog-wide state after publication. Add deterministic coverage for atomic publication and snapshot immutability. ### Release note Prevent connector consumers from modifying shared catalog storage adapter snapshots. ### Check List (For Author) - Test: Unit Test (`CatalogPropertyTest`) - Behavior changed: No. This hardens the existing snapshot contract. - Does this need documentation: No
3 participants
What problem does this PR solve?
Concurrent first consumers of a catalog's Hadoop properties could observe the cache before its initialization completed. One thread could copy the
HashMapwhile the initializer was still populating it, causing aConcurrentModificationExceptionduring external table sink binding.What is changed?
Validation
CatalogPropertyTestPaimonWriteBindingTestStoragePropertiesFsCacheFingerprintTest