Skip to content

[Improvement-17480][Storage] Separate the local storage implementation from hdfs storage plugin #17547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ruanwenjun merged 2 commits into from
Oct 9, 2025
Merged

[Improvement-17480][Storage] Separate the local storage implementation from hdfs storage plugin #17547

ruanwenjun merged 2 commits into from
Oct 9, 2025

Conversation

@ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Sep 28, 2025 •
edited
Loading

Purpose of the pull request

close #17480
close #17542
close #17459

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 28, 2025
View reviewed changes
@SbloodyS SbloodyS added this to the 3.3.2 milestone Sep 29, 2025
@ruanwenjun ruanwenjun marked this pull request as draft September 29, 2025 01:44
@ruanwenjun ruanwenjun added the improvement make more easy to user or prompt friendly label Sep 29, 2025
@ruanwenjun ruanwenjun marked this pull request as ready for review September 29, 2025 02:08
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch 2 times, most recently from c4fcf45 to 413f399 Compare September 29, 2025 05:57
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch 3 times, most recently from 3b8abf1 to 983927b Compare September 29, 2025 14:52
@github-actions github-actions bot added the e2e e2e test label Sep 29, 2025
@github-actions github-actions bot added e2e e2e test and removed e2e e2e test labels Sep 29, 2025
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch 2 times, most recently from 22e4f86 to 48dbdaf Compare September 30, 2025 02:12
@github-actions github-actions bot removed the e2e e2e test label Sep 30, 2025
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch 3 times, most recently from 27e1b18 to 0626b76 Compare September 30, 2025 02:15
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch 3 times, most recently from 1f96990 to 327d9e9 Compare September 30, 2025 05:42
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 30, 2025
View reviewed changes
...src/main/java/org/apache/dolphinscheduler/plugin/storage/api/local/LocalStorageOperator.java
if (exists(directoryAbsolutePath)) {
throw new FileAlreadyExistsException("Directory already exists: " + directoryAbsolutePath);
}
Files.createDirectories(path);

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
@ruanwenjun
Copy link
Member Author

ruanwenjun commented Sep 30, 2025

We have check the path in api, so the storage plugin don't need to check it again.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch from eccf67c to ec3ef80 Compare October 4, 2025 02:45
@SbloodyS
Copy link
Member

SbloodyS commented Oct 5, 2025

CI failed. @ruanwenjun

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch from ec3ef80 to 3be8605 Compare October 8, 2025 06:36
@github-actions github-actions bot added the e2e e2e test label Oct 8, 2025
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 8, 2025
View reviewed changes
...src/main/java/org/apache/dolphinscheduler/plugin/storage/api/local/LocalStorageOperator.java
if (FileUtils.isDirectory(srcFile)) {
FileUtils.copyDirectoryToDirectory(srcFile, dstFile);
} else {
FileUtils.copyFile(srcFile, dstFile);

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
This path depends on a
user-provided value
Loading
.
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_fix17480 branch from 3be8605 to c7c6fec Compare October 8, 2025 08:24
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 8, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
57.5% Coverage on New Code (required ≥ 60%)

See analysis details on SonarQube Cloud

SbloodyS
SbloodyS approved these changes Oct 8, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ruanwenjun ruanwenjun merged commit 7235f25 into apache:dev Oct 9, 2025
69 of 72 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_fix17480 branch October 9, 2025 09:23
davidzollo pushed a commit to davidzollo/dolphinscheduler that referenced this pull request Oct 27, 2025
@ruanwenjun @davidzollo
[Improvement-17480][Storage] Separate the local storage implementatio…
9c50536 
…n from hdfs storage plugin (apache#17547)
@abangv
Copy link

abangv commented Dec 22, 2025

`dolphinscheduler-storage-plugin/dolphinscheduler-storage-api/src/main/java/org/apache/dolphinscheduler/plugin/storage/api/AbstractStorageOperator.java

protected void exceptionIfPathNotUnderStorageBaseDir(String resourceAbsolutePath) {
    String storageBaseDirectory = getStorageBaseDirectory();
    if (!resourceAbsolutePath.startsWith(storageBaseDirectory)) {
        throw new IllegalArgumentException(
                "Resource path: " + resourceAbsolutePath + " is not under storage base directory: "
                        + storageBaseDirectory);
    }
}

`method exceptionIfPathNotUnderStorageBaseDir unused.where used?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhongjiajie zhongjiajie zhongjiajie approved these changes

@SbloodyS SbloodyS SbloodyS approved these changes

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

Assignees

@ruanwenjun ruanwenjun

Labels

backend e2e e2e test improvement make more easy to user or prompt friendly priority:high test

Projects

None yet

Milestone

3.3.2

4 participants

@ruanwenjun @SbloodyS @abangv @zhongjiajie