[doc](fe) Add fe-filesystem module README and translate ENV_TEST_README to English#62511
[doc](fe) Add fe-filesystem module README and translate ENV_TEST_README to English#62511morningman wants to merge 2 commits intoapache:masterfrom
Conversation
…ME to English Issue Number: close #xxx Problem Summary: The fe-filesystem module lacked English documentation explaining its plugin architecture, module structure, and how to add new filesystem sub-modules. The existing ENV_TEST_README.md was in Chinese only. This commit adds: 1. A comprehensive README.md covering module purpose, plugin loading mechanism, relationship to other modules, and step-by-step guide for adding new backends. 2. English translation of ENV_TEST_README.md (environment test documentation). None - Test: No need to test (documentation only) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
skip buildall |
|
/review |
github-actions
bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found 1 blocking issue in this docs-only PR.
fe/fe-filesystem/README.mddocuments the plugin runtime layout incorrectly: it says each subdirectory underfilesystem_plugin_rootshould contain a plugin zip, butDirectoryPluginRuntimeManageractually scans each subdirectory as an unpacked plugin directory and only loads*.jarpluslib/*.jarfrom inside it. Following the current README would produce a plugin directory with no root jar, and plugin discovery would fail.
Critical checkpoint conclusions:
- Goal of the task: add English FE filesystem module documentation and environment-test docs. The PR mostly achieves that, but the new README is not yet correct enough to be safely used as an implementation guide because the plugin deployment contract is misstated. No code or doc validation test was added.
- Scope/minimality: focused and small; only documentation files are touched.
- Concurrency: not applicable to this PR; no executable code changed.
- Lifecycle/static init: not applicable to this PR; no lifecycle code changed.
- Configuration items: no new config added. Existing
filesystem_plugin_rootis documented, but one runtime detail is inaccurate. - Compatibility/incompatible changes: none.
- Parallel code paths: not applicable beyond matching the README against the existing plugin-loader path, which is where the issue was found.
- Special conditionals: not applicable.
- Test coverage: no tests were added, which is acceptable for docs-only changes, but it also means the README instructions were not exercised end-to-end.
- Test result changes: none.
- Observability: not applicable.
- Transaction/persistence: not applicable.
- Data writes/modifications: not applicable.
- FE-BE variable passing: not applicable.
- Performance: no runtime impact from the patch itself.
- Other issues: none beyond the incorrect plugin deployment guidance above.
Summary opinion: request changes until the README matches the real plugin loading behavior.
| classpath (built-in providers, test overrides). | ||
| 2. **Directory plugin scan** — Uses `DirectoryPluginRuntimeManager` to scan the directory | ||
| configured by `Config.filesystem_plugin_root` (default: `${DORIS_HOME}/plugins/filesystem`). | ||
| Each subdirectory is expected to contain a plugin zip produced by the Maven assembly plugin. |
There was a problem hiding this comment.
DirectoryPluginRuntimeManager does not consume zip files from each plugin subdirectory. It treats each direct child of filesystem_plugin_root as the already-unpacked plugin directory and then resolves pluginDir/*.jar + pluginDir/lib/*.jar from there (fe-extension-loader/.../DirectoryPluginRuntimeManager.java). If someone follows this sentence literally and drops doris-fe-filesystem-gcs.zip into ${DORIS_HOME}/plugins/filesystem/gcs/, discovery fails with No jar found under plugin directory. Please reword this to describe the unpacked runtime layout, and keep the zip only as the build artifact that gets unzipped before deployment.
…y layout ### What problem does this PR solve? Issue Number: close #xxx Problem Summary: The README incorrectly stated that DirectoryPluginRuntimeManager loads plugin zip files. It actually expects unpacked directories with *.jar at root and lib/*.jar for dependencies. Dropping a .zip file would cause "No jar found under plugin directory" errors. ### Release note None ### Check List (For Author) - Test: No need to test (documentation only) - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
github-actions
bot
removed
the
approved
There was a problem hiding this comment.
One blocking documentation issue remains.
fe/fe-filesystem/ENV_TEST_README.md: the direct Maven examples and the later note use-Dsurefire.excludedGroups, but thefe-filesystem/pom.xmlwiring reads${test.excludedGroups}into Surefire's<excludedGroups>. As written, the documented commands still leavetest.excludedGroups=environmentin effect, so users following this README will continue to skip all environment-tagged tests instead of enabling them.
Critical checkpoint conclusions:
- Goal of current task: Partially achieved. The PR adds the requested English documentation and the new README is largely aligned with the implementation, but the environment-test instructions are not actually executable as documented.
- Modification size/focus: Yes. The PR is small and documentation-focused.
- Concurrency: Not applicable for this docs-only PR.
- Lifecycle/static initialization: Not applicable.
- Configuration items: No new config is added, but existing config/property behavior is documented incorrectly in one place.
- Incompatible changes: None.
- Functionally parallel code paths: Applicable. The helper script and the POM already document/use
test.excludedGroups; the direct Maven path in this README should match them. - Special conditional checks: Not applicable.
- Test coverage: No code tests required for docs-only changes, but documentation must still be validated against the real invocation path. That validation currently fails for the direct Maven examples.
- Test result modifications: None.
- Observability: Not applicable.
- Transaction/persistence/data write/new FE-BE variables: Not applicable.
- Performance: Not applicable.
- Other issues: None beyond the blocking doc mismatch above.
Please update the README examples/notes to use -Dtest.excludedGroups=none (or otherwise document the actual property consumed by the build).
| @@ -90,7 +111,7 @@ mvn test -pl fe-filesystem \ | |||
There was a problem hiding this comment.
fe-filesystem/pom.xml does not read a surefire.excludedGroups property. It sets Surefire's <excludedGroups> from ${test.excludedGroups} instead, and the default is environment. That means the command shown here still skips every env test, so the README's direct-Maven instructions do not work as documented. Please switch these examples (and the note below) to -Dtest.excludedGroups=none, which is what the module POM and run-fs-env-test.sh actually rely on.
2 participants
No description provided.