ozone
ozone copied to clipboard
HDDS-10781. Do not use OFSPath in O3FS BasicOzoneClientAdapterImpl
What changes were proposed in this pull request?
In BasicOzoneClientAdapterImpl#isFileClosed we use OFSPath to check if the path was a key, but it Utility class for OFS not O3FS. That is to say, the pathStr doen't include volume and bucket but the OFSPath still setting the first two levels of directories or key as volume and bucket. If the path was less than two level it comes out an error.
https://github.com/apache/ozone/blob/HDDS-7593/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L774
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10781
How was this patch tested?
There are some existing lease recovery tests.
Thanks @chungen0126 for the fix.
Why is this targeted at feature branch HDDS-7593
? isFileClosed
is already on master
, it was added for HDDS-8436. It should be fixed on master
first.
Also, can you please check if similar usage of OFSPath
related to snapshots should/can be replaced?
https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L601
https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L610
https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L620
https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L665
Why is this targeted at feature branch
HDDS-7593
?isFileClosed
is already onmaster
, it was added for HDDS-8436. It should be fixed onmaster
first.
Should I close this pr and create a new one for master?
Also, can you please check if similar usage of
OFSPath
related to snapshots should/can be replaced?https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L601
https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L610
https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L620
https://github.com/apache/ozone/blob/fe1b5b63353819acf393398b64ab4152c7e8d51f/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L665
I fixed those APIs make them create/rename snapshots for the root bucket and add test for them. I'm confused that should them have permission to access snapshots for other buckets.
Should I close this pr and create a new one for master?
Not necessary. If you agree with the move to master
, the PR's base branch can be changed.
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request
Looks correct to me. Can you add a small test here
https://github.com/apache/ozone/blob/21068a3871f4ddebc1bf2780813e5e72eaea2ab7/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTestWithFSO.java#L498
I put the source in the second layer folder to test for this bug.
Should I close this pr and create a new one for master?
Not necessary. If you agree with the move to
master
, the PR's base branch can be changed.https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request
Sure.
@chungen0126 sorry, looks like GitHub does not realize only 2 commits are targeted at master
:
* d76c7ef733 add test
* 6180d45b79 [hsync] BasicOzoneClientAdapterImpl#isFileClosed using OFSPath which is an Utility class for OFS not O3FS
Can you please rebase these onto master
and push to your fork? Let me know if you need help with that.
Can you please rebase these onto master and push to your fork? Let me know if you need help with that.
Done. Am I doing the right thing?
@adoroszlai Sorry, I might have done something wrong. Could you please tell me how to fix it?
@chungen0126
Start interactive rebase:
git rebase --interactive origin/master
Delete all commits from the editor except:
d76c7ef733 add test
6180d45b79 [hsync] BasicOzoneClientAdapterImpl#isFileClosed using OFSPath which is an Utility class for OFS not O3FS
Save and exit the editor.
Verify the log:
git log --oneline --decorate | head
should be something like:
57423154ba (HEAD -> HDDS-10781) add test
7cc1c0c89a [hsync] BasicOzoneClientAdapterImpl#isFileClosed using OFSPath which is an Utility class for OFS not O3FS
8d2569da59 (origin/master, origin/HEAD, master) HDDS-10097. Intermittent ManagedChannel not shutdown properly in TestWatchForCommit (#6620)
4e9dc2faae HDDS-10798. OMLeaderNotReadyException exception on switch leader (#6626)
...
Should I force push or push a new branch for new pull request, thanks?
@chungen0126 force push is better in this case than a new PR
Thanks a lot @chungen0126 for updating the patch to cover snapshot-specific methods and rebasing for master
.
Thanks @chungen0126 for the fix, @jojochuang for the review.