HDDS-10781. Do not use OFSPath in O3FS BasicOzoneClientAdapterImpl

[chungen0126]

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.

[jojochuang]

Looks correct to me. Can you add a small test here

ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTestWithFSO.java

Line 498 in 21068a3

public void testLeaseRecoverable() throws Exception {

[jojochuang]

like:

// verify that fs.isFileClosed(new Path("")) throws an exception
assertThrows(OMException.class, () -> fs.isFileClosed(new Path("")));

[adoroszlai]

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?

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 601 in fe1b5b6

OFSPath ofsPath = new OFSPath(pathStr, config);

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 610 in fe1b5b6

OFSPath ofsPath = new OFSPath(pathStr, config);

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 620 in fe1b5b6

OFSPath ofsPath = new OFSPath(pathStr, config);

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 665 in fe1b5b6

OFSPath snapPath = new OFSPath(snapshotDir.toString(), config);

[chungen0126]

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?

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 601 in fe1b5b6

OFSPath ofsPath = new OFSPath(pathStr, config);

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 610 in fe1b5b6

OFSPath ofsPath = new OFSPath(pathStr, config);

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 620 in fe1b5b6

OFSPath ofsPath = new OFSPath(pathStr, config);

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java

Line 665 in fe1b5b6

OFSPath snapPath = new OFSPath(snapshotDir.toString(), config);

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.

[adoroszlai]

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

[chungen0126]

Looks correct to me. Can you add a small test here

ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTestWithFSO.java

Line 498 in 21068a3

public void testLeaseRecoverable() throws Exception {

I put the source in the second layer folder to test for this bug.

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

[adoroszlai]

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

[chungen0126]

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]

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

[adoroszlai]

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

[chungen0126]

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

[adoroszlai]

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

[adoroszlai]

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

[jojochuang]

Thanks for the update! Almost ready to go. Just two more small nits

[adoroszlai]

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