-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
[improve][offload] Use filesystemURI as the storage path #23591
[improve][offload] Use filesystemURI as the storage path #23591
Conversation
--- ### Motivation We provided the fileSystemUri in the offload policy as the filesystem offload configuration. The fileSystemUri will overwrite the fs.defaultFS. We should use it as the storage path not the hadoop.tmp.dir as the storage path.
@zymap Please add the following content to your PR description and select a checkbox:
|
c5fd8f0
to
4b5b803
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
(cherry picked from commit b915f6e)
(cherry picked from commit b915f6e)
(cherry picked from commit b915f6e)
This change is related to #23411. I'll revert the change in branch-3.0 since the TestFileSystemOffload test fails and blocks 3.0.8 release. branch-3.0 doesn't include #23411 . |
@lhotari I don't understand why a configuration change will related to a dependency change. Shouldn't we check why the test is failed? |
…ath (apache#23591)"" This reverts commit bb909e6.
@lhotari I fixed the issue and pushed a commit to run the pulsar test here. The root cause is after this fix the fileSystemURI is working which makes the integration tests fail due to the permission issue. Because it used the root path. After changing it to the /pulsar/data, it runs successfully in my local. |
@zymap The difference in branch-3.0 is that #23411 isn't included. That's why the test failure in branch-3.0 was most likely related to that change. Something simply behaves differently. |
@zymap thanks! Wouldn't it be useful to submit similar test improvements to master branch? |
@zymap The comment referenced the test failure, https://github.com/apache/pulsar/actions/runs/11977750391/job/33404574103#step:12:11625
That problem got resolved by reverting. @zymap How did you fix that problem? |
…ath (apache#23591)"" This reverts commit bb909e6. (cherry picked from commit 720135b)
…ath (apache#23591)"" This reverts commit bb909e6. (cherry picked from commit 720135b)
Motivation
Fixes #xyz
Main Issue: #xyz
PIP: #xyz
Motivation
We provided the fileSystemUri in the offload policy as the filesystem offload configuration. The fileSystemUri will overwrite the fs.defaultFS. We should use it as the storage path not the hadoop.tmp.dir as the storage path.
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: