You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/06/17 20:52:19 UTC
[GitHub] [hadoop-ozone] smengcl opened a new pull request #1088: HDDS-3805. [OFS] Use ClientProtocol directly in Adapter and FS
smengcl opened a new pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088
## 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl edited a comment on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-652243600
`TestFailureHandlingByClient#testPipelineExclusionWithPipelineFailure` looks like a flaky test. The same test suite passed on my [fork](https://github.com/smengcl/hadoop-ozone/runs/819477863).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl edited a comment on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl edited a comment on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-652243600
`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](https://github.com/smengcl/hadoop-ozone/runs/819477863).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-652248876
@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 `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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-652243600
`TestFailureHandlingByClient#testPipelineExclusionWithPipelineFailure` looks like a flaky test. The same test suite passed on my [fork]https://github.com/smengcl/hadoop-ozone/runs/819477863).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
arp7 commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-679270871
Folks - what is the benefit of splitting this into smaller Jiras? Is it just to make the code review easier?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-671916540
/pending Yeah I will spilt the change into more jiras.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1088: HDDS-3805. [OFS] Use ClientProtocol directly in Adapter and FS
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-647761078
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl closed pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl closed pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1088: HDDS-3805. [OFS] Use ClientProtocol directly in Adapter and FS
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-646825797
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-679986318
> 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-679346944
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-659250951
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-665142666
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] smengcl commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-682863419
> > 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)](https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-659250951)) and suggested some changes.
>
> And would be interested about the discussion/decision/plan about the proposed changes.
I am closing this PR and opening https://github.com/apache/hadoop-ozone/pull/1363.
Let's continue the discussion there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] arp7 commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
arp7 commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-680296917
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-679984177
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
elek commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-681859652
> 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] (https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-659250951) and suggested some changes.
And would be interested about the discussion/decision/plan about the proposed changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on pull request #1088: HDDS-3805. [OFS] Remove usage of OzoneClientAdapter interface
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1088:
URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-670195139
> Yeah I will spilt the change into more jiras.
I have created one (HDDS-4074) for the introduction of `RootedOzFs`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org