github-actions[bot] commented on PR #61269:
URL: https://github.com/apache/doris/pull/61269#issuecomment-4071609126
## Code Review Summary
**PR: [refactor](oss) unify FE OSS filesystem with Jindo**
This PR unifies the FE-side OSS Hadoop filesystem implementation to Jindo
FS, replacing the legacy `AliyunOSSFileSystem` and `paimon-oss` dependencies.
The changes are clean, focused, and well-tested.
### Critical Checkpoint Conclusions
1. **Goal and correctness**: The PR accomplishes its stated goal of
switching FE OSS filesystem to Jindo FS. Both `OSSProperties` and
`OSSHdfsProperties` now consistently use Jindo FS class names via shared
constants. Existing tests (OSSPropertiesTest, OSSHdfsPropertiesTest) and a new
test (`testUseJindoFsForHadoopStorageConfig`) verify the behavior.
2. **Modification scope**: The change is minimal and focused — only the
necessary files are touched: two Java source files, four pom.xml files, one
test file, and one regression test.
3. **Concurrency**: No concurrency concerns — the modified code paths
involve object construction and configuration initialization, not shared
mutable state.
4. **Lifecycle management**: No lifecycle concerns. The Jindo FS constants
are package-private static finals, appropriate for their usage scope.
5. **Configuration items**: No new configuration items are added. The change
is transparent to users — the same user-facing properties (`oss.endpoint`,
`oss.access_key`, etc.) continue to work; only the underlying filesystem
implementation class changes.
6. **Compatibility / rolling upgrades**: This is an FE-only change that
affects which Hadoop filesystem class is instantiated. Jindo FS was already on
the FE classpath (loaded from `lib/jindofs/` at startup). No protocol or
storage format changes. Rolling upgrades are safe since FE nodes independently
resolve filesystem implementations.
7. **Parallel code paths**: Verified that all FE code paths setting
`fs.oss.impl` now point to Jindo. No stale references to `AliyunOSSFileSystem`
remain in FE. The broker module (`fs_brokers/`) still uses the old
implementation, but the PR description explicitly scopes this to FE only, which
is appropriate.
8. **Regression test fix**: The change from `'fs.oss.support'` to
`'fs.oss-hdfs.support'` in `oss_hdfs_catalog_test.groovy` is correct — the test
exercises OSS-HDFS (JindoData DLS) properties, and `fs.oss-hdfs.support` routes
to `OSSHdfsProperties` while the old `fs.oss.support` would incorrectly route
to `OSSProperties`.
9. **Dependency removals**: Verified that `paimon-oss` has no remaining
references anywhere in the codebase, and `hadoop-aliyun` has no remaining
references in FE pom files. Clean removal.
10. **Region null safety**: The newly added
`hadoopStorageConfig.set("fs.oss.region", region)` is safe — `region` is
guaranteed non-null by validation in
`AbstractS3CompatibleProperties.initNormalizeAndCheckProps()` which throws
before `initializeHadoopStorageConfig()` is called.
11. **Test coverage**: The new unit test
`testUseJindoFsForHadoopStorageConfig` verifies the Jindo FS impl, abstract FS
impl, and region are correctly set. Existing OSSHdfsPropertiesTest already
validates Jindo constants with hardcoded string assertions (not constant
references), providing independent verification.
12. **Performance**: No performance concerns. Configuration initialization
is not a hot path.
13. **Observability**: No observability changes needed for this refactoring.
### Minor Pre-existing Note (not blocking)
`OSSProperties.initializeHadoopStorageConfig()` does not propagate
`sessionToken` as `fs.oss.securityToken` to the Hadoop config, meaning STS
temporary credentials won't work with Jindo FS via the native `oss://` path.
This is a pre-existing gap (also present in `OSSHdfsProperties` and peer
implementations like COS/OBS), not introduced by this PR. Consider addressing
it in a follow-up.
**Verdict: No issues found. The PR is clean and ready.**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
