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

[smengcl]

What changes were proposed in this pull request?

Use ClientProtocol directly in Adapter and FS.

Refactor OFS. Part 1 of N.

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]

The META-INF change seems to be affecting the MR acceptance test:

Caused by: org.apache.hadoop.fs.UnsupportedFileSystemException: fs.AbstractFileSystem.ofs.impl=null: No AbstractFileSystem configured for scheme: ofs
	at org.apache.hadoop.fs.AbstractFileSystem.createFileSystem(AbstractFileSystem.java:169)
	at org.apache.hadoop.fs.AbstractFileSystem.get(AbstractFileSystem.java:258)

Looking into it.

[elek]

Thanks to create this patch @smengcl

There are multiple changes in this patch:

1. start to use SPI / META-INF services --> I am +1

2. Change the defaultFs from o3fs to ofs in some tests --> I can accept it, I guess because we think the ofs is a long term solution, but don't know the motivation to do it now. But I can accept it.

3. Use ClientProtocol (which supposed to be an internal implementation detail of OzoneClientAdapter instead of using the ClientAdapterImpl.

I think it's a good step to remove the usage of implementation as the main goal with interfaces is to hide the implementation.

But it's not clear what is the long-term goal with this change.

A. We can either remove the usage of ClientAdapter from RootedOzoneFileSystem and use just the pure OzoneClient / ClientProtocol. In that case we don't need to leak the ClientProtocol from the ClientAdapter.

B. Or we can keep the OzoneClientAdapter if we need a good interface. Using interface means that we don't cast it and don't retrieve internal implementation, just call the methods without understanding what is under the hood. In this case the getVolumeDetails should be added to the OzoneClientAdapter instead of makeing it possible to get the ClientProtocol

I am fine with both approaches but I would like to understand the goal.

[smengcl]

Thanks @elek for the review.
See inline:

Thanks to create this patch @smengcl

There are multiple changes in this patch:

I'll revise the jira title to more accurately reflect my changes in a bit.

1. start to use SPI / META-INF services --> I am +1
2. Change the defaultFs from o3fs to ofs in some tests --> I can accept it, I guess because we think the ofs is a long term solution, but don't know the motivation to do it now. But I can accept it.

The defaultFS doesn't seem to affect any existing tests right now since I believe most are using full path (e.g. o3fs://bucket1.volume1/key1 in mapreduce.robot).

My idea here is to let docker dev experience use OFS by default. So you can do things like ozone sh mkdir -p /volume1/bucket2/.

If some tests are using relative path and tried to create files directly under volume or bucket with OFS, some acceptance tests should fail and catch the problem.

3. Use ClientProtocol (which supposed to be an internal implementation detail of OzoneClientAdapter instead of using the ClientAdapterImpl.

I think it's a good step to remove the usage of implementation as the main goal with interfaces is to hide the implementation.

But it's not clear what is the long-term goal with this change.

A. We can either remove the usage of ClientAdapter from RootedOzoneFileSystem and use just the pure OzoneClient / ClientProtocol. In that case we don't need to leak the ClientProtocol from the ClientAdapter.

B. Or we can keep the OzoneClientAdapter if we need a good interface. Using interface means that we don't cast it and don't retrieve internal implementation, just call the methods without understanding what is under the hood. In this case the getVolumeDetails should be added to the OzoneClientAdapter instead of makeing it possible to get the ClientProtocol

I am fine with both approaches but I would like to understand the goal.

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

[smengcl]

TestFailureHandlingByClient#testPipelineExclusionWithPipelineFailure looks like a flaky test.

[ERROR] Tests run: 5, Failures: 1, Errors: 0, Skipped: 1, Time elapsed: 252.16 s <<< FAILURE! - in org.apache.hadoop.ozone.client.rpc.TestFailureHandlingByClient
[ERROR] testPipelineExclusionWithPipelineFailure(org.apache.hadoop.ozone.client.rpc.TestFailureHandlingByClient)  Time elapsed: 96.773 s  <<< FAILURE!
java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:86)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.junit.Assert.assertTrue(Assert.java:52)
	at org.apache.hadoop.ozone.client.rpc.TestFailureHandlingByClient.testPipelineExclusionWithPipelineFailure(TestFailureHandlingByClient.java:411)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)

The same test suite it-client passed on my fork.

[smengcl]

@elek
I've added a few commits since you last reviewed. I have removed OFS classes' dependency on OzoneClientAdapter, and added new methods in the Impl so it doesn't leak proxy (ClientProtocol) to BasicRootedOzoneFileSystem anymore.

I am considering merging/rearranging BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl (was BasicRootedOzoneClientAdapterImpl) but each are 850+ lines long so I might do this in another jira.

Now that all the existing tests have passed. Mind taking another look at this?

[elek]

Thanks the updated @smengcl. And sorry for the later answer. I was ooo.

There are multiple changes in this patch:

I'll revise the jira title to more accurately reflect my changes in a bit.

Still, it contains 3 changes (META-INF changes, defaultFs=ofs change, code structure refactor). You can make the review easier (and faster) by separating them to different patches.

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?

[smengcl]

@elek Yeah I will spilt the change into more jiras.
Yes it would be a good approach to merge those two classes into one, since there is no point in having them both now.

[adoroszlai]

Yeah I will spilt the change into more jiras.

I have created one (HDDS-4074) for the introduction of RootedOzFs.

[elek]

/pending Yeah I will spilt the change into more jiras.

[github-actions]

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Yeah I will spilt the change into more jiras.

[arp7]

Folks - what is the benefit of splitting this into smaller Jiras? Is it just to make the code review easier?

[smengcl]

After a short discussion with @arp7 and @adoroszlai I think for this PR we won't merge the two classes (BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl). I'm opening another jira for this.

[elek]

Folks - what is the benefit of splitting this into smaller Jiras? Is it just to make the code review easier?

1. Make the code review easier
2. Make it easier to detect problems
3. Make it easier to revert commits in case of any problems.

As an example: changing o3fs to ofs in acceptance tests are independent of the Java interface changes. It may or may not introduce new problems on own. But it's easy to create a separated PR and check the build results and merge.

After that it's easier to identify possible problems, if it appeared about o3fs -> ofs change, it can be related to tests. If it's appeared as a result of the interface changes, we know that the java code should be adjusted.

[elek]

After a short discussion with @arp7 and @adoroszlai I think for this PR we won't merge the two classes (BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl). I'm opening another jira for this.

I am fine with any approach as I wrote, but can you please share some motivations behind the decision?

[arp7]

Can we defer the class merge to a separate PR? This PR has been open for over 2 months. In the interests of time, I propose to commit what we can and address the rest in a followup, or alternatively just abandon the PR.

[elek]

Can we defer the class merge to a separate PR? This PR has been open for over 2 months. In the interests of time, I propose to commit what we can and address the rest in a followup, or alternatively just abandon the PR.

I totally agree, this is what I suggested: to merge different parts in different PRs to make it easier to follow the changes.

As far as I see I see the following parts which are unrelated to the class hierarchy change:

1. Creating RootFS is already merged
2. using META-INF services --> we have a PR
3. changing default fs --> we can create the PR

So we have the remaining part which is mainly cleaning up the class hierarchy. I had a comment at [July of 16th] (#1088 (comment)) and suggested some changes.

And would be interested about the discussion/decision/plan about the proposed changes.

[smengcl]

Can we defer the class merge to a separate PR? This PR has been open for over 2 months. In the interests of time, I propose to commit what we can and address the rest in a followup, or alternatively just abandon the PR.

I totally agree, this is what I suggested: to merge different parts in different PRs to make it easier to follow the changes.

As far as I see I see the following parts which are unrelated to the class hierarchy change:

1. Creating RootFS is already merged
2. using META-INF services --> we have a PR
3. changing default fs --> we can create the PR

So we have the remaining part which is mainly cleaning up the class hierarchy. I had a comment at [July of 16th] (#1088 (comment)) and suggested some changes.

And would be interested about the discussion/decision/plan about the proposed changes.

I am closing this PR and opening #1363.

Let's continue the discussion there.