ozone icon indicating copy to clipboard operation
ozone copied to clipboard

HDDS-10781. Do not use OFSPath in O3FS BasicOzoneClientAdapterImpl

Open chungen0126 opened this issue 9 months ago • 12 comments

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.

chungen0126 avatar Apr 30 '24 23:04 chungen0126

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

adoroszlai avatar May 01 '24 04:05 adoroszlai

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.

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.

chungen0126 avatar May 02 '24 18:05 chungen0126

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

adoroszlai avatar May 02 '24 19:05 adoroszlai

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.

chungen0126 avatar May 02 '24 19:05 chungen0126

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 avatar May 02 '24 20:05 chungen0126

@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.

adoroszlai avatar May 03 '24 07:05 adoroszlai

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?

chungen0126 avatar May 03 '24 20:05 chungen0126

@adoroszlai Sorry, I might have done something wrong. Could you please tell me how to fix it?

chungen0126 avatar May 03 '24 20:05 chungen0126

@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)
...

adoroszlai avatar May 03 '24 20:05 adoroszlai

Should I force push or push a new branch for new pull request, thanks?

chungen0126 avatar May 04 '24 09:05 chungen0126

@chungen0126 force push is better in this case than a new PR

adoroszlai avatar May 04 '24 09:05 adoroszlai

Thanks a lot @chungen0126 for updating the patch to cover snapshot-specific methods and rebasing for master.

adoroszlai avatar May 04 '24 10:05 adoroszlai

Thanks @chungen0126 for the fix, @jojochuang for the review.

adoroszlai avatar May 09 '24 20:05 adoroszlai