HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface

[smengcl]

This PR is derived from #1088 as the previous one is split into multiple jiras and needs rebase. I'm hoping opening up a brand new PR for the original jira would make things simpler.

What changes were proposed in this pull request?

Use ClientProtocol directly in Adapter and FS.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3805

How was this patch tested?

Existing tests should pass.

[smengcl]

Note the previous 5 commits are directly from the previous PR. The last commit 37a4414 is to address the new getTrashRoots impl.

I wanted to use proxy directly, only to find that I would need to build what ObjectStore has already done for me when listing volumes. So I kept ObjectStore as class global variable.

[elek]

Thanks for the separation @smengcl, I think it's easier to discuss.

I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped:

I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then.

I am fine with that approach but let me add some comments to the latest patch.

The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation)

As a test: Can you please explain what are the differences between the two classes and the responsibilities?

If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem.

Wouldn't it be more simple?

This patch seems to use BasicRootedOzoneClientAdapterImpl which is not an Impl (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need Impl in the name (it doesn't implement anything)?

[smengcl]

Thanks for the separation @smengcl, I think it's easier to discuss.

I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped:

I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then.

I am fine with that approach but let me add some comments to the latest patch.

The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation)

As a test: Can you please explain what are the differences between the two classes and the responsibilities?

If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem.

Wouldn't it be more simple?

This patch seems to use BasicRootedOzoneClientAdapterImpl which is not an Impl (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need Impl in the name (it doesn't implement anything)?

I'm thinking of renaming BasicRootedOzoneClientAdapterImpl to BasicRootedOzoneFileSystemHelper. This way the class name should make more sense?

But one uncertainly is that there is one class extending BasicRootedOzoneClientAdapterImpl:

public class RootedOzoneClientAdapterImpl
    extends BasicRootedOzoneClientAdapterImpl {

The class is dealing with OzoneFSStorageStatistics, but not used anywhere. The o3fs counterpart is OzoneClientAdapterImpl, which is used in OzoneFileSystem.

I'm not sure of the implication of the renaming. What do you think?

[elek]

I'm thinking of renaming BasicRootedOzoneClientAdapterImpl to BasicRootedOzoneFileSystemHelper. This way the class name should make more sense?

Definitely better, IMHO. It's not clear why we need to move out some functions to a helper class ("Can you please explain what are the differences between the two classes and the responsibilities?"), but I can live with it, just to merge the patch earlier (not a big deal).

The class is dealing with OzoneFSStorageStatistics, but not used anywhere. The o3fs counterpart is OzoneClientAdapterImpl, which is used in OzoneFileSystem.

This is only about the statistics. Independent to what type of classes do you have:

You need a OzoneFSStorageStatistics storageStatistics which is updated for each of the operations, but only for Hadoop3 (!!!).

For the old-school o3fs it's done by subclasses:

• Default implementation is hadoop2 compatibility as incdementCounter does nothing
• non-Basic implementation increments a real counter.

As far as I see we already have this logic for ofs as the imlementation of RootedOzoneFileSystem in hadoop2 and hadoop3 projects are different. (Later one update the statistics.)

If your helper class can be used from both project without problem, you don't need to create two helper classses for the two use-cases.

[elek]

/pending

[github-actions]

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

/pending

[github-actions]

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."