[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 |
2 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